[Cocoa] Add an ImageDecoder subclass backed by AVFoundation
Created attachment 320903 [details] Patch
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.
(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.
Created attachment 321100 [details] Patch for landing
Created attachment 321107 [details] Patch for landing
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.
Created attachment 321115 [details] Patch for landing
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.
Created attachment 321133 [details] Patch
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.
Created attachment 321142 [details] Patch for landing
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.
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
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
Created attachment 321199 [details] Patch for landing
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
Created attachment 321200 [details] Patch for landing
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.
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
Created attachment 321213 [details] Patch for landing
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.
Created attachment 321215 [details] Patch for landing
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.
Created attachment 321229 [details] Patch for landing
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.
Comment on attachment 321229 [details] Patch for landing Clearing flags on attachment: 321229 Committed r222225: <http://trac.webkit.org/changeset/222225>
All reviewed patches have been landed. Closing bug.
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.
(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.
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.
<rdar://problem/34693404>