Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pro Photo Upload Settings #759

Merged
merged 54 commits into from
Sep 24, 2020
Merged

Pro Photo Upload Settings #759

merged 54 commits into from
Sep 24, 2020

Conversation

mneuwert
Copy link
Contributor

@mneuwert mneuwert commented Jul 27, 2020

Description

Added two photo upload settings in Settings -> Media Upload:

  • Prefer original photos (original photo instead of edited version is picked when uploading)
  • Prefer RAW (RAW variant is chosen instead of JPEG version, both are usually created by apps like Halide when shooting RAW)

This feature is meant to be added to the same IAP product as EXIF meta-data

Related Issue

#685
#688

Motivation and Context

For users serious about photography we would like to add more flexibility when uploading photos shot on iPhone.

How Has This Been Tested?

1. Upload original vs. edited photo

  • Open settings menu in oC App, open "Media upload" sub-menu, activate "Prefer original photos" option.
  • Shoot a picture with a stock camera app
  • Edit a photo in the Photos app or any other app and e.g. apply "Noir" filter to make the picture B/W
  • Open a bookmark, navigate to desired sub-folder and tap + to upload from photo library
  • Choose edited photo and upload
  • Original photo without applied effects shall be uploaded

2. Upload RAW photos

  • Open settings menu in oC App, open "Media upload" sub-menu, activate "Prefer RAW photos" option.
  • Shoot a picture with an app able to shoot RAW pictures (e.g. Halide, Manual etc.). Make sure RAW mode is active in the app you have chosen.
  • Open a bookmark, navigate to desired sub-folder and tap + to upload from photo library
  • Make sure .DNG file is uploaded containing RAW pixel information instead of JPG

Screenshots (if appropriate):

IMG_C7685F93A550-1

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.

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/iOS-app/Version%2011.4.1/PhotoUploadsIAP.md

Bugs & improvements:

Michael Neuwert and others added 25 commits July 1, 2020 16:29
# Conflicts:
#	ownCloud.xcodeproj/project.pbxproj
- Progress indicator while data is being generated
- Fixed parsing of ISO speed rating (film sensitivity)
- Not displaying meta-data where value is empty
- Prefer uploading originals (non-edited)
- Prefer uploading RAW images
@CLAassistant
Copy link

CLAassistant commented Jul 27, 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.
1 out of 3 committers have signed the CLA.

✅ hosy
❌ 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.

@michaelstingl
Copy link
Contributor

  • Open a bookmark, navigate to desired sub-folder and tap + to upload from photo library
  • Make sure .DNG file is uploaded containing RAW pixel information instead of JPG

This works very nice!

Could we highlight RAW images already in the image picker?

IMG_5C00B2FE77E5-1

@mneuwert
Copy link
Contributor Author

@michaelstingl Yes, absolutely.. Although I didn't explicitly mentioned RAW in issue #510, this is what I had in mind.. But we can add a badge for RAW photos in scope of this PR already.

@mneuwert
Copy link
Contributor Author

@michaelstingl Added RAW photo badge in the upload picker UI

Copy link
Contributor

@felix-schwarz felix-schwarz left a comment

Choose a reason for hiding this comment

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

Thanks for making changes. I think bookmarksChanged needs to wrap the call to reconsiderProPhotoSettingsSection in OnMainThread. However, it would still not update in a multi-window use case when accounts are added or removed, though.

Normally, I'd agree that responding dynamically is an edge case, but since license observers make this so easy to implement, I'd go for it. Here's a version that's fully dynamic (and also shows and hides AutoUploadSettingsSection as needed):

//
//  MediaUploadSettingsViewController.swift
//  ownCloud
//
//  Created by Michael Neuwert on 22.05.20.
//  Copyright © 2020 ownCloud GmbH. All rights reserved.
//

/*
* Copyright (C) 2020, ownCloud GmbH.
*
* This code is covered by the GNU Public License Version 3.
*
* For distribution utilizing Apple mechanisms please see https://owncloud.org/contribute/iOS-license-exception/
* You should have received a copy of this license along with this program. If not, see <http://www.gnu.org/licenses/gpl-3.0.en.html>.
*
*/

import UIKit
import ownCloudSDK
import ownCloudApp
import ownCloudAppShared

class MediaUploadSettingsViewController: StaticTableViewController {

	private var proPhotoSettingsSection: StaticTableViewSection?
	private var autoUploadSection : AutoUploadSettingsSection?
	private var licenseObserver : OCLicenseObserver?

	deinit {
		NotificationCenter.default.removeObserver(self, name: .OCBookmarkManagerListChanged, object: nil)
	}

	override func viewDidLoad() {
		super.viewDidLoad()
		navigationItem.title = "Media Upload".localized

		if let userDefaults = OCAppIdentity.shared.userDefaults {
			proPhotoSettingsSection = ProPhotoUploadSettingsSection(userDefaults: userDefaults)
			self.addSection(MediaExportSettingsSection(userDefaults: userDefaults))
		}

		NotificationCenter.default.addObserver(self, selector: #selector(reconsiderSections), name: .OCBookmarkManagerListChanged, object: nil)

		licenseObserver = OCLicenseManager.shared.observeProducts(nil, features: [ .photoProFeatures ], in: OCLicenseEnvironment(), withOwner: self) { [weak self] (_, _, _) in
			self?.reconsiderSections()
		}
	}

	@objc private func reconsiderSections() {
		OnMainThread {
			guard let proSettingsSection = self.proPhotoSettingsSection else { return }

			if OCBookmarkManager.shared.bookmarks.count > 0 {
				if self.autoUploadSection == nil, let userDefaults = OCAppIdentity.shared.userDefaults {
					self.autoUploadSection = AutoUploadSettingsSection(userDefaults: userDefaults)
				}

				if let autoUploadSection = self.autoUploadSection, !autoUploadSection.attached {
					self.addSection(autoUploadSection)
				}
				// TODO: Re-add this section when we re-gain an ability to run background NSURLSessions
				//self.addSection(BackgroundUploadsSettingsSection(userDefaults: userDefaults))
			} else {
				if let autoUploadSection = self.autoUploadSection, autoUploadSection.attached {
					self.removeSection(autoUploadSection)
					self.autoUploadSection = nil
				}
			}

			if OCLicenseEnterpriseProvider.numberOfEnterpriseAccounts > 0 || OCLicenseManager.shared.authorizationStatus(forFeature: .photoProFeatures, in: OCLicenseEnvironment()) == .granted {
				if !proSettingsSection.attached {
					self.addSection(proSettingsSection)
				}
			} else {
				if proSettingsSection.attached {
					self.removeSection(proSettingsSection)
				}
			}
		}
	}
}

@mneuwert
Copy link
Contributor Author

@felix-schwarz ok, makes sense.. agreed and merged your suggestion

@jesmrec
Copy link
Contributor

jesmrec commented Sep 22, 2020

(1) [wont fix]

Probably not bug, but a feature:

Options to select are in the Settings view, so, they are not under the control of the card to be enabled/disabled. Check these steps up:

  1. Add an enterprise account to the device
  2. Add a non-enterprise account to the device (feature not purchased)
  3. Open Settings > Media upload

Current: Options are there and available for both accounts. By enabling the Prefer unedited , both accounts will upload the unedited version of the pictures.

Is this behaviour the expected one?

iPhoneXR iOS13.7
oC server 10.5

@jesmrec
Copy link
Contributor

jesmrec commented Sep 22, 2020

(2) [OK]

To test the RAW feature, i'm using a free app called RAW+ that is free for a maximum number of shots. Not sure if this is a proper app, because i see different behaviours that do not match the expected ones:

  • In the albums, the RAW badge mentioned here is not there.

  • iPhoneXR: no matter if i enable or disable the Prefer RAW option, the file is always uploaded in DNG format. I also have an iPadAIr (older), which uploads always produce a JPG file, no matter the Setting (probably due to an older camera)

Did you test with other devices/apps? which result did you get?

@mneuwert
Copy link
Contributor Author

Re (1)

@jesmrec That's "works as designed". The settings are not tied to the account. Also, if user buys a feature as IAP it should work with all accounts, right?

@jesmrec
Copy link
Contributor

jesmrec commented Sep 22, 2020

Re (1)

@jesmrec That's "works as designed". The settings are not tied to the account. Also, if user buys a feature as IAP it should work with all accounts, right?

yes, you're right. You purchase a feature and it is available everywhere. But, the mentioned case is not about purchasing a feature (at least by the user itself). Not sure if this make sense... i will write an example:

I work for owncloud, so i have an enterprise account in the corporate server that allows me to use all pro-features.
Then, i add and account in my own server, that is not enterprise. So, in my app with these two attached servers:

  • Scanner, Shortcuts and Markup: i can use them in enterprise account, but not in my account because i did not purchase anything.
  • Pro uploads: i can use it in both accounts, even though i have not purchased it.

is this corner case expected and accepted as feature (not bug)?

CC @michaelstingl @hosy

@michaelstingl
Copy link
Contributor

is this corner case expected and accepted as feature (not bug)?

CC @michaelstingl @hosy

Behaviour is fine. 👍 (I don't want to make implementation too difficult – will have proper licensing API in a future backend release)

@jesmrec
Copy link
Contributor

jesmrec commented Sep 22, 2020

ok, then (1) is done.

@mneuwert
Copy link
Contributor Author

Re (2)

@jesmrec

  • Prefer RAW means: upload RAW if it is available. Photo asset may have multiple resources attached to it. So a photo can have JPG version which is going to be used as preview and RAW image.
  • Not all iOS devices have camera which is able to capture RAW.
  • Not sure if the app you used is adhering to the Apple's official recommendations summarised in the article (see link below).
  • I mostly tested with Halide app running on iPhone 11 Pro. Could also check iPhone X / iPad Pro.
  • iPhone XR: most probably RAW+ app is storing only one resource (DNG) in the photo library. Therefore since there is no choice it is picked.
  • iPad Air you used eventually doesn't support shooting RAW at all. We could add additional check and disable prefer RAW in case device's camera doesn't support RAW. WDYT?

Reference:
https://developer.apple.com/documentation/avfoundation/cameras_and_media_capture/capturing_still_and_live_photos/capturing_photos_in_raw_format

@jesmrec
Copy link
Contributor

jesmrec commented Sep 22, 2020

Thanks for the explanations.

I will try to do it with the stuff i have. If not posible, probably any member of the team can perform some tests in p2p with me.

iPad Air you used eventually doesn't support shooting RAW at all. We could add additional check and disable prefer RAW in case device's camera doesn't support RAW. WDYT?

Hidding options that are not really available is always a good UX practice.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 23, 2020

(3) [FIXED]

  1. Take a photo with Halide app
  2. In Settings, enable both Prefer unedited and Prefer RAW
  3. Upload the pic

Current: Pic uploaded in JPG
Expected: Pic uploaded in DNG

By enabling only the Prefer RAW it works fine

iPhoneXR v13.7
app 325a8e33b

Michael Neuwert added 3 commits September 23, 2020 23:56
…ro_photo_features

# Conflicts:
#	ownCloud.xcodeproj/project.pbxproj
Soe apps don’t seem to follow guidelines for storing RAW photos suggested by Apple. So instead of using alternatePhoto, they store RAW as main resource
@mneuwert
Copy link
Contributor Author

@jesmrec Addressed (2) and (3). As for (2) made detection of RAW photos in the upload selection view more tolerant. Apple suggests storing RAW as alternatePhoto resource type. RAW+ app however does store just one resource and it is main photo resource. Also when I try to setup the app to store both JPG and RAW, it only seems to store RAW version.

@mneuwert
Copy link
Contributor Author

@jesmrec Improved UX by not displaying the "Prefer RAW" option in case there is no physical camera HW which would support shooting RAW.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 24, 2020

(2) and (3) are fixed.

Tested with debug ipa build with Xcode12.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 24, 2020

Xcode compilation problems are fixed and now branch is correctly built with Xcode11.7

Approved.

@jesmrec jesmrec added the Approved by QA Approved by QA label Sep 24, 2020
@hosy hosy merged commit 037c787 into milestone/11.4.1 Sep 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/pro_photo_features branch September 24, 2020 11:40
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants