Bug 214270 – Add support for parsing VP-style codec strings.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Add support for parsing VP-style codec strings.

[Radar WebKit Bug Importer]

<rdar://problem/65500048>

[Jer Noble]

Created attachment 404173 [details]
Patch

[Darin Adler]

Comment on attachment 404173 [details]
Patch

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

> Source/WebCore/platform/graphics/VP9Utilities.cpp:37
> +static HashSet<uint8_t> validVPLevels()

HashSet is not a good way to search a short list of integers. A better pattern is implemented in the portAllowed function in the URL.cpp file.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:39
> +    static auto levels = makeNeverDestroyed(HashSet<uint8_t> {

If we stick with this we should add a const here. I think static const.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:60
> +    static auto primaries = makeNeverDestroyed(HashSet<uint8_t> {

Ditto.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:94
> +    if (!equal(configuration.codecName, "vp08") && !equal(configuration.codecName, "vp09"))

I suggest using != here.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:106
> +    configuration.profile = toIntegralType<uint8_t>(profile, &isValidProfile);

Would be much better to use the one returning Optional instead of the boolean out argument. Even if we have to add that; otherwise it’s just code we have to rewrite later. This applies many times below.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:117
> +    if (!isValidLevel || !validVPLevels().contains(configuration.level))

We need to make sure we test with empty and deleted values of the HashSet. Could avoid that issue by rewriting as I suggest above.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:153
> +    // Fifth element: colourPrimaries. Legal values are defined by  ISO/IEC 23001-8:2016, superceded

We shouldn't use the British colour in WebKit code. Why would we do that here?

> Source/WebCore/platform/graphics/VP9Utilities.cpp:158
> +    if (!isValidColourPrimaries || !validColourPrimaries().contains(configuration.colourPrimaries))

We need to make sure we test with empty and deleted values of the HashSet. Could avoid that issue by rewriting as I suggest above.

> Source/WebCore/platform/graphics/VP9Utilities.h:28
> +#include <wtf/Vector.h>

This include should not be needed.

> Source/WebCore/platform/graphics/VP9Utilities.h:42
> +    uint8_t profile;
> +    uint8_t level;
> +    uint8_t bitDepth;
> +    uint8_t chromaSubsampling;
> +    uint8_t videoFullRangeFlag;
> +    uint8_t colourPrimaries;
> +    uint8_t transferCharacteristics;
> +    uint8_t matrixCoefficients;

I suggest we initialize these to 0.

> Source/WebCore/platform/graphics/VP9Utilities.h:45
> +WEBCORE_EXPORT Optional<VPCodecConfigurationRecord> parseVPCodecParameters(const String& codecString);

This should take a StringView, not a String.

> Source/WebCore/testing/Internals.idl:204
> +    unsigned short profile;
> +    unsigned short level;
> +    unsigned short bitDepth;
> +    unsigned short chromaSubsampling;
> +    unsigned short videoFullRangeFlag;
> +    unsigned short colourPrimaries;
> +    unsigned short transferCharacteristics;
> +    unsigned short matrixCoefficients;

These say unsigned short, but our enum says uint8_t. Those are different types.

> LayoutTests/media/vp-codec-parameters.html:45
> +        testExpected('internals.parseVPCodecParameters("bad-parameter")', null);
> +        testExpected('internals.parseVPCodecParameters("vp09")', null);
> +        testExpectedVPCodecConfigurationRecord('internals.parseVPCodecParameters("vp09.00.41.08")', makeVPCodecConfigurationRecord('vp09', 0, 41, 8, 1, 1, 1, 1, 0));
> +        testExpectedVPCodecConfigurationRecord('internals.parseVPCodecParameters("vp09.02.10.10.01.09.16.09.01")', makeVPCodecConfigurationRecord('vp09', 2, 10, 10, 1, 9, 16, 9, 1));
> +        testExpected('internals.parseVPCodecParameters("vp10.00.41.08")', null);
> +        testExpected('internals.parseVPCodecParameters("vp09.00.41.08.01.01.01.00.00")', null);

I’d like to see way more test cases. A separate one for every check we do in the function above. That should add maybe 20 more test cases.

[Jer Noble]

(In reply to Darin Adler from comment #3)
> Comment on attachment 404173 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404173&action=review
> 
> > Source/WebCore/platform/graphics/VP9Utilities.cpp:37
> > +static HashSet<uint8_t> validVPLevels()
> 
> HashSet is not a good way to search a short list of integers. A better
> pattern is implemented in the portAllowed function in the URL.cpp file.

Oh! A sorted binary search! Much better; will adopt.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:94
> > +    if (!equal(configuration.codecName, "vp08") && !equal(configuration.codecName, "vp09"))
> 
> I suggest using != here.

Ok.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:106
> > +    configuration.profile = toIntegralType<uint8_t>(profile, &isValidProfile);
> 
> Would be much better to use the one returning Optional instead of the
> boolean out argument. Even if we have to add that; otherwise it’s just code
> we have to rewrite later. This applies many times below.

Okay, we'll need to add it, but it shouldn't be difficult.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:153
> > +    // Fifth element: colourPrimaries. Legal values are defined by  ISO/IEC 23001-8:2016, superceded
> 
> We shouldn't use the British colour in WebKit code. Why would we do that
> here?

Only because that's literally the spelling in the VP9 specification.  But sure, we can Americanize the spelling.

> > Source/WebCore/platform/graphics/VP9Utilities.h:28
> > +#include <wtf/Vector.h>
> 
> This include should not be needed.

Removed.

> > Source/WebCore/platform/graphics/VP9Utilities.h:42
> > +    uint8_t profile;
> > +    uint8_t level;
> > +    uint8_t bitDepth;
> > +    uint8_t chromaSubsampling;
> > +    uint8_t videoFullRangeFlag;
> > +    uint8_t colourPrimaries;
> > +    uint8_t transferCharacteristics;
> > +    uint8_t matrixCoefficients;
> 
> I suggest we initialize these to 0.

Ok.

> > Source/WebCore/platform/graphics/VP9Utilities.h:45
> > +WEBCORE_EXPORT Optional<VPCodecConfigurationRecord> parseVPCodecParameters(const String& codecString);
> 
> This should take a StringView, not a String.

Ok.

> > Source/WebCore/testing/Internals.idl:204
> > +    unsigned short profile;
> > +    unsigned short level;
> > +    unsigned short bitDepth;
> > +    unsigned short chromaSubsampling;
> > +    unsigned short videoFullRangeFlag;
> > +    unsigned short colourPrimaries;
> > +    unsigned short transferCharacteristics;
> > +    unsigned short matrixCoefficients;
> 
> These say unsigned short, but our enum says uint8_t. Those are different
> types.

TIL about "octet" in WebIDL.

> > LayoutTests/media/vp-codec-parameters.html:45
> > +        testExpected('internals.parseVPCodecParameters("bad-parameter")', null);
> > +        testExpected('internals.parseVPCodecParameters("vp09")', null);
> > +        testExpectedVPCodecConfigurationRecord('internals.parseVPCodecParameters("vp09.00.41.08")', makeVPCodecConfigurationRecord('vp09', 0, 41, 8, 1, 1, 1, 1, 0));
> > +        testExpectedVPCodecConfigurationRecord('internals.parseVPCodecParameters("vp09.02.10.10.01.09.16.09.01")', makeVPCodecConfigurationRecord('vp09', 2, 10, 10, 1, 9, 16, 9, 1));
> > +        testExpected('internals.parseVPCodecParameters("vp10.00.41.08")', null);
> > +        testExpected('internals.parseVPCodecParameters("vp09.00.41.08.01.01.01.00.00")', null);
> 
> I’d like to see way more test cases. A separate one for every check we do in
> the function above. That should add maybe 20 more test cases.

Ok.

[Sam Weinig]

Comment on attachment 404173 [details]
Patch

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

> Source/WebCore/testing/Internals.idl:195
> +] dictionary VPCodecConfigurationRecord {

Will this be used beyond these tests? Any reason to not just use TestWebKitAPI and test the interface directly? Seems like that would be more straightforward.

[Jer Noble]

Created attachment 404205 [details]
Patch

[Jer Noble]

Created attachment 404206 [details]
Patch

[Jer Noble]

Created attachment 404209 [details]
Patch

[Jer Noble]

(In reply to Sam Weinig from comment #5)
> Comment on attachment 404173 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404173&action=review
> 
> > Source/WebCore/testing/Internals.idl:195
> > +] dictionary VPCodecConfigurationRecord {
> 
> Will this be used beyond these tests? Any reason to not just use
> TestWebKitAPI and test the interface directly? Seems like that would be more
> straightforward.

No, and that's why it's there. We can't test the interface directly. This parser isn't exposed directly to JS. It's used by MediaCapabilities, but even then you can't disambiguate between "this property isn't supported by the codec" and "this property parsed badly", hence this test.

[Eric Carlson]

Comment on attachment 404209 [details]
Patch

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

> Source/WebCore/platform/graphics/VP9Utilities.cpp:161
> +    if (!transferCharacteristics || (!*transferCharacteristics || *transferCharacteristics == 3 || *transferCharacteristics > 18))
> +        return WTF::nullopt;

As discussed, 2 is undefined.

> LayoutTests/media/vp-codec-parameters.html:130
> +        consoleWrite('');
> +        consoleWrite('Test invalid videoFullRangeFlag:');
> +        testExpected('internals.parseVPCodecParameters("vp09.02.10.10.01.09.16.01.02")', null);
> +        endTest();

Your parser fails if there is anything after the videoFullRangeFlag, so you might as well test that as well.

[Jer Noble]

(In reply to Eric Carlson from comment #10)
> Comment on attachment 404209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404209&action=review
> 
> > Source/WebCore/platform/graphics/VP9Utilities.cpp:161
> > +    if (!transferCharacteristics || (!*transferCharacteristics || *transferCharacteristics == 3 || *transferCharacteristics > 18))
> > +        return WTF::nullopt;
> 
> As discussed, 2 is undefined.

Good catch!

> > LayoutTests/media/vp-codec-parameters.html:130
> > +        consoleWrite('');
> > +        consoleWrite('Test invalid videoFullRangeFlag:');
> > +        testExpected('internals.parseVPCodecParameters("vp09.02.10.10.01.09.16.01.02")', null);
> > +        endTest();
> 
> Your parser fails if there is anything after the videoFullRangeFlag, so you
> might as well test that as well.

That's tested in the last entry of "Test invalid number of optional parameters".

[Jer Noble]

Created attachment 404250 [details]
Patch for landing

[Darin Adler]

Comment on attachment 404250 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/VP9Utilities.cpp:36
> +    static const uint16_t validLevels[] = {

constexpr uint8_t

> Source/WebCore/platform/graphics/VP9Utilities.cpp:57
> +static bool isValidVPcolorPrimaries(uint8_t colorPrimarary)

"primarary"

Is it "primaries" or "primary"? This calls it both.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:59
> +    static const uint16_t validcolorPrimaries[] = {

Ditto.

Why is "color" lowercase here?

> Source/WebCore/platform/graphics/VP9Utilities.cpp:77
> +Optional<VPCodecConfigurationRecord> parseVPCodecParameters(const StringView& codecString)

Should just be StringView, not const StringView&.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:85
> +    StringView codecView(codecString);

This is strange, both are StringView.

> Source/WebCore/platform/graphics/VP9Utilities.cpp:133
> +        configuration.chromaSubsampling = 1;
> +        configuration.colorPrimaries = 1;
> +        configuration.transferCharacteristics = 1;
> +        configuration.matrixCoefficients = 1;

Could initialize to 1 instead of 0 and get rid of this?

> Source/WebCore/platform/graphics/VP9Utilities.cpp:134
> +        configuration.videoFullRangeFlag = 0;

Not needed, since we initialize to 0.

[Darin Adler]

Comment on attachment 404250 [details]
Patch for landing

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

>> Source/WebCore/platform/graphics/VP9Utilities.cpp:59
>> +    static const uint16_t validcolorPrimaries[] = {
> 
> Ditto.
> 
> Why is "color" lowercase here?

By "Ditto" I meant:

static constexpr uint8_t

[Jer Noble]

(In reply to Darin Adler from comment #13)
> Comment on attachment 404250 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404250&action=review
> 
> > Source/WebCore/platform/graphics/VP9Utilities.cpp:36
> > +    static const uint16_t validLevels[] = {
> 
> constexpr uint8_t

Ok.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:57
> > +static bool isValidVPcolorPrimaries(uint8_t colorPrimarary)
> 
> "primarary"
> 
> Is it "primaries" or "primary"? This calls it both.

Yeah, this is confusing. Each enumerated value is a single number that represents a trio of color primaries. But they're a single enumerated value, so they feel like they should be singular, and a group of multiple enumerated values should be plural. Having them both be plural leads to a "1 fish, 2 fish" situation. But that's probably more correct anyway.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:59
> > +    static const uint16_t validcolorPrimaries[] = {
> 
> Ditto.
> 
> Why is "color" lowercase here?

Whoops, bad search-replace.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:77
> > +Optional<VPCodecConfigurationRecord> parseVPCodecParameters(const StringView& codecString)
> 
> Should just be StringView, not const StringView&.

Ok.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:85
> > +    StringView codecView(codecString);
> 
> This is strange, both are StringView.

Will remove.

> > Source/WebCore/platform/graphics/VP9Utilities.cpp:133
> > +        configuration.chromaSubsampling = 1;
> > +        configuration.colorPrimaries = 1;
> > +        configuration.transferCharacteristics = 1;
> > +        configuration.matrixCoefficients = 1;
> 
> Could initialize to 1 instead of 0 and get rid of this?

I didn't want to necessarily bake the default values into the header, but I don't have a good justification for that, so I'll just do that.

[Jer Noble]

Created attachment 404254 [details]
Patch for landing

[EWS]

Committed r264367: <https://trac.webkit.org/changeset/264367>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404254 [details].

[Sam Weinig]

(In reply to Jer Noble from comment #9)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 404173 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404173&action=review
> > 
> > > Source/WebCore/testing/Internals.idl:195
> > > +] dictionary VPCodecConfigurationRecord {
> > 
> > Will this be used beyond these tests? Any reason to not just use
> > TestWebKitAPI and test the interface directly? Seems like that would be more
> > straightforward.
> 
> No, and that's why it's there. We can't test the interface directly. This
> parser isn't exposed directly to JS. It's used by MediaCapabilities, but
> even then you can't disambiguate between "this property isn't supported by
> the codec" and "this property parsed badly", hence this test.

Right, I am asking why you don't test the parser directly in TestWebKitAPI, which is the way we can test C/C++ interfaces directly. It doesn't seem necessary to expose this to JS at all.