tilltue / TLPhotoPicker

📷 multiple phassets picker for iOS lib. like a facebook
MIT License
1.88k stars 332 forks source link

tempCopyMediaFile() and live photo #230

Open giofid opened 5 years ago

giofid commented 5 years ago

I am not sure that tempCopyMediaFile() function is entirely correct when the asset is a live photo and convertLivePhotosToJPG is false. In this case we obtain a .MOV file but the content of data (returned by PHImageManager.default().requestImageData()) is a jpeg. So we 'll get a video that's not a video.

tilltue commented 5 years ago

Hi, @giofid You right. It was bug, i've fixed that and push now. https://github.com/tilltue/TLPhotoPicker/commit/e1ea38e1389094bcffc830c5a4a3afecfab48b13

giofid commented 5 years ago

Hi, @tilltue In my app I'm implementing a function like tempCopyMediaFile() to copy an image/video from photo library into Documents app folder. I'm a little confused about Photos framework so I take this opportunity to ask you if it's possible to use PHAssetResourceManager.default().requestData() or PHAssetResourceManager.default().writeData() regardless to PHAssetResourceType. At line 190 you already have the right asset resource. Why can't you just use that to request or write its data?

....
guard let resource = (PHAssetResource.assetResources(for: phAsset).filter { $0.type == type }).first else { }
let fileName = resource.originalFilename
let options = PHAssetResourceRequestOptions()
options.isNetworkAccessAllowed = true
var imageData = Data()
PHAssetResourceManager.default().requestData(for: resource, options: options, dataReceivedHandler: { (data) in
    imageData.append(data)
}, completionHandler: { (error) in
    DispatchQueue.main.async {
        if error != nil {
            // show error message
        } else {
            // write imageData to fileName file into Documents folder
        }
    }
})
.....

Thanks for your help.

wade-hawk commented 5 years ago

Hi, @giofid As you know, If the asset is 'icloud photos', you need to download it.

You right. There are options for download in 'PHAssetResourceManager.default().writeData' ( isNewtorkAccessAllowed in 'PHAssetResourceRequestOptions')

It's not much different than writeData after requestLivePhoto.

But I wanted to provide more contextual options. That's why it using 'PHImageManager.default().requestLivePhoto' functions. ( isNetworkAccessAllowed options in PHLivePhotoRequestOptions )

It was a function provided for convenience, so I thought it would be good to have more useful.

giofid commented 5 years ago

Thank you @tilltue, @wade-hawk!! I take advantage of your help to submit you two other things:

tilltue commented 5 years ago

PNG Case

스크린샷 2019-11-01 오전 12 29 07

HEIC Case

스크린샷 2019-11-01 오전 12 30 24

JPEG Case

스크린샷 2019-11-01 오전 12 31 01

@giofid I can't understand why should renamed file in this function? I think if you need to convert png to jpg should making custom functions.

giofid commented 5 years ago

Hi @tilltue, to reproduce the extension mismatch you have to edit a png image into your Photo Library.

  1. Import a png image into your Photo Library;
  2. Edit it. For example write some text over it. Save it;
  3. Try your demo.

Schermata 2019-10-31 alle 17 20 09

During this test you'll also see that computed var originalFileName implementation is not entirely correct. With edited photo, in the first position of PHAssetResource.assetResources() array you'll find an Adjustment.plist file

public var originalFileName: String? {
      get {
          guard let phAsset = self.phAsset,let resource = PHAssetResource.assetResources(for: phAsset).first else { return nil }
          return resource.originalFilename
      }
}
ghost commented 4 years ago

A similar issue happens if you try to export a non-live HEIC photo - the resulting file has a JPG extension but the file itself is still HEIC. Of course, it's not a problem for live photos if convertLivePhotosToJPG is true.