Bug 176825 – [Cocoa] Add an ImageDecoder subclass backed by AVFoundation

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] Add an ImageDecoder subclass backed by AVFoundation

[Jer Noble]

Created attachment 320903 [details]
Patch

[Eric Carlson]

Comment on attachment 320903 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:66
> +SOFT_LINK_FRAMEWORK(AVFoundation)
> +SOFT_LINK_CLASS(AVFoundation, AVURLAsset)
> +SOFT_LINK_CLASS(AVFoundation, AVSampleBufferGenerator)
> +SOFT_LINK_CLASS(AVFoundation, AVSampleBufferRequest)
> +SOFT_LINK_CONSTANT(AVFoundation, AVMediaCharacteristicVisual, AVMediaCharacteristic)
> +SOFT_LINK_CONSTANT(AVFoundation, AVURLAssetReferenceRestrictionsKey, NSString *)

These should be SOFT_LINK_OPTIONAL_* because AVFoundation is not available on a recovery partition.

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:427
> +    return m_sampleData.isEmpty() ? EncodedDataStatus::Unknown : EncodedDataStatus::Complete;

Nit: I believe early return is the preferred style for a short method like this:

if (!m_sampleData.isEmpty())
    return EncodedDataStatus::Complete;

return EncodedDataStatus::Unknown;

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:432
> +    return m_size ? m_size.value() : IntSize();

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:460
> +    return m_size ? m_size.value() : IntSize();

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:465
> +    return index < m_sampleData.size() ? m_sampleData[index].image : false;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:475
> +    return index < m_sampleData.size() ? m_sampleData[index].duration : 0;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:480
> +    return index < m_sampleData.size() ? m_sampleData[index].hasAlpha : false;

Ditto.

[Jer Noble]

(In reply to Eric Carlson from comment #2)
> Comment on attachment 320903 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320903&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:66
> > +SOFT_LINK_FRAMEWORK(AVFoundation)
> > +SOFT_LINK_CLASS(AVFoundation, AVURLAsset)
> > +SOFT_LINK_CLASS(AVFoundation, AVSampleBufferGenerator)
> > +SOFT_LINK_CLASS(AVFoundation, AVSampleBufferRequest)
> > +SOFT_LINK_CONSTANT(AVFoundation, AVMediaCharacteristicVisual, AVMediaCharacteristic)
> > +SOFT_LINK_CONSTANT(AVFoundation, AVURLAssetReferenceRestrictionsKey, NSString *)
> 
> These should be SOFT_LINK_OPTIONAL_* because AVFoundation is not available
> on a recovery partition.

Oooh, good point.

> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:427
> > +    return m_sampleData.isEmpty() ? EncodedDataStatus::Unknown : EncodedDataStatus::Complete;
> 
> Nit: I believe early return is the preferred style for a short method like
> this:
> 
> if (!m_sampleData.isEmpty())
>     return EncodedDataStatus::Complete;
> 
> return EncodedDataStatus::Unknown;

Ok.

> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:432
> > +    return m_size ? m_size.value() : IntSize();
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:460
> > +    return m_size ? m_size.value() : IntSize();
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:465
> > +    return index < m_sampleData.size() ? m_sampleData[index].image : false;
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:475
> > +    return index < m_sampleData.size() ? m_sampleData[index].duration : 0;
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:480
> > +    return index < m_sampleData.size() ? m_sampleData[index].hasAlpha : false;
> 
> Ditto.

Ok on all.

[Jer Noble]

Created attachment 321100 [details]
Patch for landing

[Jer Noble]

Created attachment 321107 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 22 files


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

[Jer Noble]

Created attachment 321115 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 22 files


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

[Jer Noble]

Created attachment 321133 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


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

[Jer Noble]

Created attachment 321142 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


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

[Build Bot]

Comment on attachment 321142 [details]
Patch for landing

Attachment 321142 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4588695

New failing tests:
fast/images/animated-image-mp4.html

[Build Bot]

Created attachment 321159 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Jer Noble]

Created attachment 321199 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321199 [details]
Patch for landing

Rejecting attachment 321199 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 321199, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4594214

[Jer Noble]

Created attachment 321200 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


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

[WebKit Commit Bot]

Comment on attachment 321200 [details]
Patch for landing

Rejecting attachment 321200 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
normal/x86_64/ImageDocument.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/html/ImageDocument.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/ImageDocument.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/ImageDecoder.o platform/graphics/ImageDecoder.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/4594283

[Jer Noble]

Created attachment 321213 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


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

[Jer Noble]

Created attachment 321215 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


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

[Jer Noble]

Created attachment 321229 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:83:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


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

[WebKit Commit Bot]

Comment on attachment 321229 [details]
Patch for landing

Clearing flags on attachment: 321229

Committed r222225: <http://trac.webkit.org/changeset/222225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Layout test fast/images/animated-image-mp4.html is timing out on the GTK port since it was added in r222225 "[Cocoa] Add an ImageDecoder subclass backed by AVFoundation".

I see the test is skipped on iOS. And its purpose is a bit interesting:

description('Test that an mp4 media file loaded as an image can be painted in a canvas.')

I have no idea why that would be desirable. Jer, is this something that should be implemented for all platforms (in which case it should be skipped in each platform's TestExpectations) or something that's Mac-specific (in which case it should be skipped in the global TestExpectations and enabled in the Mac expectations)?

In the meantime, I'll skip the test on GTK.

[Jer Noble]

(In reply to Michael Catanzaro from comment #28)
> Layout test fast/images/animated-image-mp4.html is timing out on the GTK
> port since it was added in r222225 "[Cocoa] Add an ImageDecoder subclass
> backed by AVFoundation".
> 
> I see the test is skipped on iOS. And its purpose is a bit interesting:
> 
> description('Test that an mp4 media file loaded as an image can be painted
> in a canvas.')
> 
> I have no idea why that would be desirable. Jer, is this something that
> should be implemented for all platforms (in which case it should be skipped
> in each platform's TestExpectations) or something that's Mac-specific (in
> which case it should be skipped in the global TestExpectations and enabled
> in the Mac expectations)?

There are some upcoming image formats (specifically HEIF <https://en.wikipedia.org/wiki/High_Efficiency_Image_File_Format>) whose contents are effectively a video stream. So the distinction between "this is a video file" and "this is an image file" is becoming more semantic.  Also, we've seen large-scale CDNs silently upgrade a .gif to a .mp4 and (where supported) deliver the .mp4 version to clients without changing the <image src=".gif"> markup.

I'm also going to try to make this compatible with iOS as well; there is an API problem on that platform, but we want to make this work there as well.

> In the meantime, I'll skip the test on GTK.

[Michael Catanzaro]

I hope you're not seriously considering supporting HEIF? Consider whether that would be evil. There would be tremendous harm to open source web engines if Safari were to support a patent-encumbered image format. We're already having enough trouble with evil MP4 video; if HEIF takes off and we lose the ability to display images for the next decade or two, that's going to be pretty demotivating.

[Radar WebKit Bug Importer]

<rdar://problem/34693404>