Bug 172787 – Allow clients to specify a list of codecs which should require hardware decode support.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Allow clients to specify a list of codecs which should require hardware decode support.

[Jer Noble]

Created attachment 311660 [details]
Patch

[Jer Noble]

rdar://problem/31483572

[Build Bot]

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.

[Jer Noble]

Created attachment 311664 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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?

[Jer Noble]

(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.

[Alex Christensen]

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.

[Jer Noble]

Created attachment 312016 [details]
Patch

[Build Bot]

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.

[Jer Noble]

(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).

[Jer Noble]

Created attachment 312022 [details]
Patch

[Alex Christensen]

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.

[Jer Noble]

Created attachment 312031 [details]
Patch for landing

[Jer Noble]

Created attachment 312033 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312033 [details]
Patch for landing

Clearing flags on attachment: 312033

Committed r217799: <http://trac.webkit.org/changeset/217799>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.