Bug 145539 – [iOS] Uploading an animated GIF from the photo library uploads a JPEG

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Jon Honeycutt]

Uploading an animated GIF from the photo library uploads a JPEG version of the image.

To reproduce:

1. Go to <http://i.imgur.com/I8HyAQD.gif>
2. Long-press on the image, and select “Save Image”
3. Go to <http://imgur.com> and tap “upload” then “device”
4. Select the animated GIF from step 1, and select “start upload”

Image uploaded is a single frame JPEG.

<rdar://problem/19760877>

[Jon Honeycutt]

Created attachment 254100 [details]
Patch

[WebKit Commit Bot]

Attachment 254100 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:838:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 254100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254100&action=review

Overall looks good to me. I had some small comments / questions. Ultimately needs a WK2 owner.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:786
> +        // FIXME: Should we get the photo asset and get the actual filename for the photo instead of
> +        // naming each of the individual uploads image.jpg? This won't work for photos taken with
> +        // the camera, but would work for photos picked from the library.

See <rdar://problem/12353008>. Can we do this now in the asset URL path?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:792
> +static NSString *uploadImageNameFromUTI(NSString *uti)

This seems easy to trip up, if there is any UTI photo library supports that we don't support. We should be able to go from a UTI directly to a file extension with UTTypeCopyPreferredTagWithClass:
https://developer.apple.com/library/ios/documentation/MobileCoreServices/Reference/UTTypeRef/#//apple_ref/c/func/UTTypeCopyPreferredTagWithClass

Something like:

    CFStringRef extension = UTTypeCopyPreferredTagWithClass((CFStringRef)uti, kUTTagClassFilenameExtension);

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:829
> +    PHFetchResult *result = [getPHAssetClass() fetchAssetsWithALAssetURLs:@[assetURL] options:nil];
> +    if (!result.count) {
> +        LOG_ERROR("WKFileUploadPanel: Failed to fetch asset with URL %@", assetURL);
> +        [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> +        return;
> +    }

Should this fetch be done on the background queue as well, could it take a while?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:846
> +        PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager];
> +        [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) {
> +            if (!imageData) {
> +                LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL);
> +                [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> +                return;
> +            }
> +            NSString *imageName = uploadImageNameFromUTI(dataUTI);
> +            [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock];
> +        }];

In this case we don't do compressions (probably necessary). Do images of these types retain geolocation metadata? The JPEG case ends up not including the metadata.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:888
> +    // If we have an asset URL, try to upload the native image.
> +    NSURL *referenceURL = [info objectForKey:UIImagePickerControllerReferenceURL];
> +    if (referenceURL) {
> +        [self _uploadItemForImage:originalImage withAssetURL:referenceURL successBlock:successBlock failureBlock:failureBlock];
>          return;
>      }

As from the previous comment. Does this referenceURL contain a better name that we can use instead of "image.ext"? For instance, maybe we can get the actual name "IMG_3373.PNG"?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:891
> +    // Photos taken with the camera will not have an asset URL. Fall back to a JPEG representation.
> +    [self _uploadItemForJPEGRepresentationOfImage:originalImage successBlock:successBlock failureBlock:failureBlock];

Ultimately it sounds like we will still need a generic name.

[Jon Honeycutt]

(In reply to comment #3)
> Comment on attachment 254100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254100&action=review
> 
> Overall looks good to me. I had some small comments / questions. Ultimately
> needs a WK2 owner.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:786
> > +        // FIXME: Should we get the photo asset and get the actual filename for the photo instead of
> > +        // naming each of the individual uploads image.jpg? This won't work for photos taken with
> > +        // the camera, but would work for photos picked from the library.
> 
> See <rdar://problem/12353008>. Can we do this now in the asset URL path?

There’s SPI for this, but no API. I’ll look into this.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:792
> > +static NSString *uploadImageNameFromUTI(NSString *uti)
> 
> This seems easy to trip up, if there is any UTI photo library supports that
> we don't support. We should be able to go from a UTI directly to a file
> extension with UTTypeCopyPreferredTagWithClass:
> https://developer.apple.com/library/ios/documentation/MobileCoreServices/
> Reference/UTTypeRef/#//apple_ref/c/func/UTTypeCopyPreferredTagWithClass
> 
> Something like:
> 
>     CFStringRef extension =
> UTTypeCopyPreferredTagWithClass((CFStringRef)uti,
> kUTTagClassFilenameExtension);

Fixed.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:829
> > +    PHFetchResult *result = [getPHAssetClass() fetchAssetsWithALAssetURLs:@[assetURL] options:nil];
> > +    if (!result.count) {
> > +        LOG_ERROR("WKFileUploadPanel: Failed to fetch asset with URL %@", assetURL);
> > +        [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> > +        return;
> > +    }
> 
> Should this fetch be done on the background queue as well, could it take a
> while?

I don’t think so, but there’s no particular reason not to move it into the block as well. Moved.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:846
> > +        PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager];
> > +        [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) {
> > +            if (!imageData) {
> > +                LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL);
> > +                [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> > +                return;
> > +            }
> > +            NSString *imageName = uploadImageNameFromUTI(dataUTI);
> > +            [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock];
> > +        }];
> 
> In this case we don't do compressions (probably necessary). Do images of
> these types retain geolocation metadata? The JPEG case ends up not including
> the metadata.

Yes, all Exif data for the image is retained including GPS data.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:888
> > +    // If we have an asset URL, try to upload the native image.
> > +    NSURL *referenceURL = [info objectForKey:UIImagePickerControllerReferenceURL];
> > +    if (referenceURL) {
> > +        [self _uploadItemForImage:originalImage withAssetURL:referenceURL successBlock:successBlock failureBlock:failureBlock];
> >          return;
> >      }
> 
> As from the previous comment. Does this referenceURL contain a better name
> that we can use instead of "image.ext"? For instance, maybe we can get the
> actual name "IMG_3373.PNG”?

No, it contains only the file extension and a UUID.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:891
> > +    // Photos taken with the camera will not have an asset URL. Fall back to a JPEG representation.
> > +    [self _uploadItemForJPEGRepresentationOfImage:originalImage successBlock:successBlock failureBlock:failureBlock];
> 
> Ultimately it sounds like we will still need a generic name.

Yeah. I’m not sure that IMG_0011 is any less generic than “image” though.

Thanks for looking at this!

[Andy Estes]

Comment on attachment 254100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254100&action=review

r=me given that Joe has also looked at it.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:65
> +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsVersionCurrent, NSString *);
> +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsResizeModeNone, NSString *);

V comes after R.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:789
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> +        // FIXME: Different compression for different devices?
> +        // FIXME: Different compression for different UIImage sizes?
> +        // FIXME: Should EXIF data be maintained?
> +        const CGFloat compression = 0.8;
> +        NSData *jpeg = UIImageJPEGRepresentation(image, compression);
> +        if (!jpeg) {
> +            LOG_ERROR("WKFileUploadPanel: Failed to create JPEG representation for image");
> +            failureBlock();
> +            return;
> +        }
> +
> +        // FIXME: Should we get the photo asset and get the actual filename for the photo instead of
> +        // naming each of the individual uploads image.jpg? This won't work for photos taken with
> +        // the camera, but would work for photos picked from the library.
> +        NSString * const kUploadImageName = @"image.jpg";
> +        [self _uploadItemForImageData:jpeg originalImage:image imageName:kUploadImageName successBlock:successBlock failureBlock:failureBlock];
> +    });

You should use a C++ lambda instead of a block, since the lambda requires you to list the variables you capture.

Not at all new to this code, and I'm not asking you to fix this right now, but I don't like that it's the responsibility of the caller of -_processMediaInfoDictionaries:successBlock:failureBlock: to dispatch back to the main thread in their success/failure blocks. I think the fact that we dispatch to a global concurrent queue should be kept an implementation detail, and the API should make the guarantee that blocks are called on the main thread.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:847
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> +        RetainPtr<PHImageRequestOptions> options = adoptNS([allocPHImageRequestOptionsInstance() init]);
> +        [options setVersion:PHImageRequestOptionsVersionCurrent];
> +        [options setSynchronous:YES];
> +        [options setResizeMode:PHImageRequestOptionsResizeModeNone];
> +
> +        PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager];
> +        [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) {
> +            if (!imageData) {
> +                LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL);
> +                [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> +                return;
> +            }
> +            NSString *imageName = uploadImageNameFromUTI(dataUTI);
> +            [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock];
> +        }];
> +    });

Same comment about C++ lambdas instead of blocks, both for the block passed to dispatch_async() and the one passed to -requestImageDataForAsset:options:resultHandler:.

[Jon Honeycutt]

(In reply to comment #5)
> Comment on attachment 254100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254100&action=review
> 
> r=me given that Joe has also looked at it.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:65
> > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsVersionCurrent, NSString *);
> > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsResizeModeNone, NSString *);
> 
> V comes after R.

Fixed.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:789
> > +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> > +        // FIXME: Different compression for different devices?
> > +        // FIXME: Different compression for different UIImage sizes?
> > +        // FIXME: Should EXIF data be maintained?
> > +        const CGFloat compression = 0.8;
> > +        NSData *jpeg = UIImageJPEGRepresentation(image, compression);
> > +        if (!jpeg) {
> > +            LOG_ERROR("WKFileUploadPanel: Failed to create JPEG representation for image");
> > +            failureBlock();
> > +            return;
> > +        }
> > +
> > +        // FIXME: Should we get the photo asset and get the actual filename for the photo instead of
> > +        // naming each of the individual uploads image.jpg? This won't work for photos taken with
> > +        // the camera, but would work for photos picked from the library.
> > +        NSString * const kUploadImageName = @"image.jpg";
> > +        [self _uploadItemForImageData:jpeg originalImage:image imageName:kUploadImageName successBlock:successBlock failureBlock:failureBlock];
> > +    });
> 
> You should use a C++ lambda instead of a block, since the lambda requires
> you to list the variables you capture.

Any and I discussed this in person. I’m going to use blocks here, because the implicit behavior of having Obj-C types retained when captured is useful and makes the code more readable.

> 
> Not at all new to this code, and I'm not asking you to fix this right now,
> but I don't like that it's the responsibility of the caller of
> -_processMediaInfoDictionaries:successBlock:failureBlock: to dispatch back
> to the main thread in their success/failure blocks. I think the fact that we
> dispatch to a global concurrent queue should be kept an implementation
> detail, and the API should make the guarantee that blocks are called on the
> main thread.

I agree. I’ll file a bug.

> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:847
> > +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> > +        RetainPtr<PHImageRequestOptions> options = adoptNS([allocPHImageRequestOptionsInstance() init]);
> > +        [options setVersion:PHImageRequestOptionsVersionCurrent];
> > +        [options setSynchronous:YES];
> > +        [options setResizeMode:PHImageRequestOptionsResizeModeNone];
> > +
> > +        PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager];
> > +        [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) {
> > +            if (!imageData) {
> > +                LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL);
> > +                [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock];
> > +                return;
> > +            }
> > +            NSString *imageName = uploadImageNameFromUTI(dataUTI);
> > +            [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock];
> > +        }];
> > +    });
> 
> Same comment about C++ lambdas instead of blocks, both for the block passed
> to dispatch_async() and the one passed to
> -requestImageDataForAsset:options:resultHandler:.

Same comment. Thanks for the review!

Landed in <http://trac.webkit.org/changeset/185241>.