Bug 176825 - [Cocoa] Add an ImageDecoder subclass backed by AVFoundation
Summary: [Cocoa] Add an ImageDecoder subclass backed by AVFoundation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 176118
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-12 22:57 PDT by Jer Noble
Modified: 2017-09-27 12:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (66.03 KB, patch)
2017-09-15 08:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (67.03 KB, patch)
2017-09-18 09:31 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (74.45 KB, patch)
2017-09-18 10:45 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (74.58 KB, patch)
2017-09-18 11:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (75.66 KB, patch)
2017-09-18 14:50 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (75.73 KB, patch)
2017-09-18 15:45 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.58 MB, application/zip)
2017-09-18 17:46 PDT, Build Bot
no flags Details
Patch for landing (79.54 KB, patch)
2017-09-19 08:54 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (79.53 KB, patch)
2017-09-19 09:02 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (79.48 KB, patch)
2017-09-19 10:49 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (79.48 KB, patch)
2017-09-19 10:57 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (79.70 KB, patch)
2017-09-19 11:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2017-09-12 22:57:58 PDT
[Cocoa] Add an ImageDecoder subclass backed by AVFoundation
Comment 1 Jer Noble 2017-09-15 08:44:42 PDT
Created attachment 320903 [details]
Patch
Comment 2 Eric Carlson 2017-09-15 09:24:16 PDT
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.
Comment 3 Jer Noble 2017-09-15 09:31:57 PDT
(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.
Comment 4 Jer Noble 2017-09-18 09:31:57 PDT
Created attachment 321100 [details]
Patch for landing
Comment 5 Jer Noble 2017-09-18 10:45:25 PDT
Created attachment 321107 [details]
Patch for landing
Comment 6 Build Bot 2017-09-18 10:46:26 PDT
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.
Comment 7 Jer Noble 2017-09-18 11:28:28 PDT
Created attachment 321115 [details]
Patch for landing
Comment 8 Build Bot 2017-09-18 11:31:29 PDT
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.
Comment 9 Jer Noble 2017-09-18 14:50:58 PDT
Created attachment 321133 [details]
Patch
Comment 10 Build Bot 2017-09-18 14:53:18 PDT
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.
Comment 11 Jer Noble 2017-09-18 15:45:44 PDT
Created attachment 321142 [details]
Patch for landing
Comment 12 Build Bot 2017-09-18 15:48:28 PDT
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 13 Build Bot 2017-09-18 17:46:09 PDT
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
Comment 14 Build Bot 2017-09-18 17:46:10 PDT
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
Comment 15 Jer Noble 2017-09-19 08:54:26 PDT
Created attachment 321199 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2017-09-19 08:55:37 PDT
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
Comment 17 Jer Noble 2017-09-19 09:02:09 PDT
Created attachment 321200 [details]
Patch for landing
Comment 18 Build Bot 2017-09-19 09:36:34 PDT
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 19 WebKit Commit Bot 2017-09-19 09:37:26 PDT
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
Comment 20 Jer Noble 2017-09-19 10:49:45 PDT
Created attachment 321213 [details]
Patch for landing
Comment 21 Build Bot 2017-09-19 10:52:07 PDT
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.
Comment 22 Jer Noble 2017-09-19 10:57:49 PDT
Created attachment 321215 [details]
Patch for landing
Comment 23 Build Bot 2017-09-19 11:00:31 PDT
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.
Comment 24 Jer Noble 2017-09-19 11:58:54 PDT
Created attachment 321229 [details]
Patch for landing
Comment 25 Build Bot 2017-09-19 11:59:56 PDT
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 26 WebKit Commit Bot 2017-09-19 14:15:50 PDT
Comment on attachment 321229 [details]
Patch for landing

Clearing flags on attachment: 321229

Committed r222225: <http://trac.webkit.org/changeset/222225>
Comment 27 WebKit Commit Bot 2017-09-19 14:15:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Michael Catanzaro 2017-09-25 11:26:52 PDT
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.
Comment 29 Jer Noble 2017-09-25 12:10:58 PDT
(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.
Comment 30 Michael Catanzaro 2017-09-25 12:30:54 PDT
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.
Comment 31 Radar WebKit Bug Importer 2017-09-27 12:30:39 PDT
<rdar://problem/34693404>