Bug 172787 - Allow clients to specify a list of codecs which should require hardware decode support.
Summary: Allow clients to specify a list of codecs which should require hardware decod...
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:
Blocks:
 
Reported: 2017-05-31 16:58 PDT by Jer Noble
Modified: 2017-06-06 19:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (30.89 KB, patch)
2017-05-31 17:06 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (31.54 KB, patch)
2017-05-31 17:32 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (46.81 KB, patch)
2017-06-05 10:55 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (48.00 KB, patch)
2017-06-05 12:03 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (48.11 KB, patch)
2017-06-05 14:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (49.75 KB, patch)
2017-06-05 15:01 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-05-31 16:58:36 PDT
Allow clients to specify a list of codecs which should require hardware decode support.
Comment 1 Jer Noble 2017-05-31 17:06:29 PDT
Created attachment 311660 [details]
Patch
Comment 2 Jer Noble 2017-05-31 17:07:02 PDT
rdar://problem/31483572
Comment 3 Build Bot 2017-05-31 17:08:20 PDT
Attachment 311660 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:823:  More than one command on the same line  [whitespace/newline] [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 4 Jer Noble 2017-05-31 17:32:18 PDT
Created attachment 311664 [details]
Patch
Comment 5 Build Bot 2017-05-31 17:35:28 PDT
Attachment 311664 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:823:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Alex Christensen 2017-06-01 09:30:38 PDT
Comment on attachment 311664 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:437
> +        if (!assetTrackMeetsHardwareDecodeRequirements(track, {'hvc1'})) {
> +            m_parsingSucceeded = false;

Why do we fail to parse with just this codec check?  What's special about this codec?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:821
> +- (NSArray<NSNumber *> *)_mediaCodecsRequiringHardwareSupport

Why NSNumber? Shouldn't this be an array of NSStrings?  The corresponding C API is.

> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:94
> +    (iokit-property-regex #"^IOGVA(.*)Decode$")

No corresponding Encode property?
Comment 7 Jer Noble 2017-06-01 11:10:51 PDT
(In reply to Alex Christensen from comment #6)
> Comment on attachment 311664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311664&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:437
> > +        if (!assetTrackMeetsHardwareDecodeRequirements(track, {'hvc1'})) {
> > +            m_parsingSucceeded = false;
> 
> Why do we fail to parse with just this codec check?  What's special about
> this codec?

That flag is checked after the append and signals an error to be fired. And what's special about this codec is that we've been asked not to decode it without hardware support, and, well, there's no hardware support.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:821
> > +- (NSArray<NSNumber *> *)_mediaCodecsRequiringHardwareSupport
> 
> Why NSNumber? Shouldn't this be an array of NSStrings?  The corresponding C
> API is.

Codecs are FourCC codes, (a uint32_t, effectively), not strings.  They're pushed through the API as strings because there's no support in WKPreferences or WebPreferenceStore for a preferences whose value is an array type.

> > Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:94
> > +    (iokit-property-regex #"^IOGVA(.*)Decode$")
> 
> No corresponding Encode property?

There's already encode support in this list.
Comment 8 Alex Christensen 2017-06-01 15:18:59 PDT
If we need to pass a FourCC code, why not just pass it as a uint32_t, or four ints, or something?  Even an NSArray<NSNumber *> seems excessive if we know the array must contain only four things and can only contain four things.  I definitely don't think we should have a version that passes an array of numbers with unspecified length and one version parsing a string.
Comment 9 Jer Noble 2017-06-05 10:55:41 PDT
Created attachment 312016 [details]
Patch
Comment 10 Build Bot 2017-06-05 10:58:17 PDT
Attachment 312016 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Settings.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Jer Noble 2017-06-05 11:11:27 PDT
(In reply to Alex Christensen from comment #8)
> If we need to pass a FourCC code, why not just pass it as a uint32_t, or
> four ints, or something?  Even an NSArray<NSNumber *> seems excessive if we
> know the array must contain only four things and can only contain four
> things.  I definitely don't think we should have a version that passes an
> array of numbers with unspecified length and one version parsing a string.

So, I took this to heart and switched the setting to take a semicolon-separated list of  Content-Types, which (can) include the codec as a parameter. I did this for the reason you gave above, but also so that other ports that use non ISO-backed containers can use this same setting (other containers don't use 4CC codes to identify codecs).
Comment 12 Jer Noble 2017-06-05 12:03:29 PDT
Created attachment 312022 [details]
Patch
Comment 13 Alex Christensen 2017-06-05 13:07:03 PDT
Comment on attachment 312022 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        - a String::split() that returns a vector (rather than taking an out-reference to a vector).

We should remove the old one in another patch.

> Source/WebCore/platform/graphics/FourCC.cpp:33
> +    auto asciiValue = stringValue.ascii();

Can this be used to represent bytes outside the range 0..127?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:436
> +        if (!assetTrackMeetsHardwareDecodeRequirements(track, {'hvc1'})) {

Apparently initializer list to const Vector& doesn't work on at least El Capitan.
Comment 14 Jer Noble 2017-06-05 14:30:28 PDT
Created attachment 312031 [details]
Patch for landing
Comment 15 Jer Noble 2017-06-05 15:01:58 PDT
Created attachment 312033 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2017-06-05 15:40:34 PDT
Comment on attachment 312033 [details]
Patch for landing

Clearing flags on attachment: 312033

Committed r217799: <http://trac.webkit.org/changeset/217799>
Comment 17 WebKit Commit Bot 2017-06-05 15:40:36 PDT
All reviewed patches have been landed.  Closing bug.