Skip to content

Conversation

@mneuwert
Copy link
Contributor

@mneuwert mneuwert commented Dec 19, 2020

Description

  • Add ability to upload slo-mo videos etc
  • Add option to allow uploading original videos

Related Issue

#847

Motivation and Context

More flexibility for photo uploads

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Michael Neuwert added 2 commits November 25, 2020 22:18
Fixes slow motion video upload and other types of video relying on applying transformation data to originally shot footage
@CLAassistant
Copy link

CLAassistant commented Dec 19, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Michael Neuwert
❌ felix-schwarz


Michael Neuwert seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mneuwert mneuwert added the feature:uploads all kind of uploads label Dec 19, 2020
@mneuwert mneuwert added this to the 11.5.0-Next milestone Dec 19, 2020
@mneuwert mneuwert self-assigned this Dec 19, 2020
@mneuwert mneuwert linked an issue Dec 19, 2020 that may be closed by this pull request
@michaelstingl
Copy link
Contributor

  • Add ability to upload slo-mo videos etc

Looks very good 👍

CleanShot 2020-12-23 at 01 38 59

@michaelstingl
Copy link
Contributor

Hm, interesting. Apple's new 10bit Dolby Vision format doesn't get re-encoded at all:

CleanShot 2020-12-23 at 01 52 11

@michaelstingl
Copy link
Contributor

I found, "Prefer original videos" is only respected when upload is initiated from inside the app. Video uploads initiated from Photos app via Share Sheet or via FileProvider always have "Preparing" phase before upload, so they get re-encoded. Add a note under the switch?

@mneuwert
Copy link
Contributor Author

@michaelstingl hm.. actually these shall be uploaded as is without any processing. It can be any binary file not just video, right?

@michaelstingl
Copy link
Contributor

Check Settings > Media upload:

“Prefer original videos” and “Prefer RAW photos” seems connected. Enabling the one, also enabled the other. Disabling results in same behavior:

B6FD2EC3-ECAC-49D2-85FF-D78277CC2082.MOV

@mneuwert
Copy link
Contributor Author

Check Settings > Media upload:

“Prefer original videos” and “Prefer RAW photos” seems connected. Enabling the one, also enabled the other. Disabling results in same behavior:

B6FD2EC3-ECAC-49D2-85FF-D78277CC2082.MOV

This is fixed with latest commit

Comment on lines +616 to +623
#if values[:BETA_APP_ICON]
# commit = last_git_commit
# short_hash = commit[:abbreviated_commit_hash] # short sha of commit
# sh "brew install librsvg"
# sh "sed -e \"s/\#version#/" + version + "/\" -e \"s/\#githash#/" + short_hash + "/\" badge.svg > badge_tmp.svg"
# sh "rsvg-convert badge_tmp.svg > badge.png"
# add_badge(custom: "fastlane/badge.png")
#end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be uncommented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented that out because it failed and prevented Bitrise builds a few weeks ago (IIRC). When putting it back in, please check whether IPAs are still being built.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed with the latest master merged inside


self.add(row: preferOriginalsRow)

if AVCaptureDevice.rawCameraDeviceAvailable() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the if clause? Raw camera support is not available on every device.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the if clause? Raw camera support is not available on every device.

What about raw files stored on devices without the latest camera? I created the file in iPhone 12 Pro Max, but did my upload testing with the iPhone 8 test device.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good point. I missed that option!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hosy that was exactly the intension to allow users upload RAW photos from their iCloud synced photo library shot on more capable device.

@hosy hosy requested a review from jesmrec January 14, 2021 09:14
@jesmrec
Copy link
Contributor

jesmrec commented Jan 15, 2021

Checked the slo-mo videos, they are correctly uploaded and played. @michaelstingl collected all relevant encoding info above.

About uploading original videos:

  • Settings option works fine. It is only visible in enterprise version servers, and under Prefer original videos IAP feature
  • Option enabled + video edited = upload video without the edit.
  • Option disabled + video edited = upload video with the edit.
  • An original video is always correctly uploaded, no matter the Setting value

About encoding, i will add a report about the edited video with and without the Setting enabled. For me it's ok, but you can take a look if any parameter is more relevant

Original Edited
Screenshot 2021-01-15 at 11 13 59 Screenshot 2021-01-15 at 11 14 21

Tested with: iPhoneXR v14.2, iPadAir v13

@jesmrec
Copy link
Contributor

jesmrec commented Jan 19, 2021

Approved

@jesmrec jesmrec added the Approved by QA Approved by QA label Jan 19, 2021
@jesmrec jesmrec merged commit df3d1fb into milestone/11.5 Jan 21, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/upload_original_video branch January 21, 2021 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved by QA Approved by QA feature:uploads all kind of uploads

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check: Upload video files without re-conversion

7 participants