[GStreamer][MSE] Trim space between codecs

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

Right now `video/webm; codecs="opus, vp9"` is not accepted (in discordance with the W3C tests) but `video/webm; codecs="opus,vp9"` is.

[Alicia Boya García]

Created attachment 323399 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 323399 [details]
Patch

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

Is this change wanted considering that it makes so many other tests fail?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:847
> +        // Codecs may be have whitespace around, e.g. when the string "vp8, vorbis" is split.

may be -> might

[Alicia Boya García]

> Is this change wanted considering that it makes so many other tests fail?

Could you elaborate? What tests is it making fail?

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Is this change wanted considering that it makes so many other tests fail?

Note that the test was marked as failing, and skipped on mac, so the current expectation file is probably bogus.

[Alicia Boya García]

If you look at the asserts closely you will notice that the new output makes
more sense.

[Carlos Alberto Lopez Perez]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 323399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323399&action=review
> 
> Is this change wanted considering that it makes so many other tests fail?
> 

I think those failures were already there.
This is what you get if you run the test with trunk :  http://sprunge.us/UDgi

What I'm unsure is about changing the expectation to contain FAIL lines. I know some WebKit developers do that, but that always looked wrong to me.

This patch makes more subtests pass, but it doesn't fix all the subtests of the test yet.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> What I'm unsure is about changing the expectation to contain FAIL lines. I
> know some WebKit developers do that, but that always looked wrong to me.

We've basically decided to do that for WPT tests, yes, because that's what Youenn prefers and he's doing all of the work.

But please not in the tests that we write ourselves.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Alberto Lopez Perez from comment #6)
> > What I'm unsure is about changing the expectation to contain FAIL lines. I
> > know some WebKit developers do that, but that always looked wrong to me.
> 
> We've basically decided to do that for WPT tests, yes, because that's what
> Youenn prefers and he's doing all of the work.
> 
> But please not in the tests that we write ourselves.

I think the one we are talking about is a WPT test.

[Carlos Alberto Lopez Perez]

Comment on attachment 323399 [details]
Patch

I'm missing here the ChangeLog for the LayoutTests directory.
I think you should re-run the tool to generate the changelogs.

[Michael Catanzaro]

Ummm yes.

Why are the expectations asserting that we don't accept these MIME types?

[Alicia Boya García]

> This patch makes more subtests pass, but it doesn't fix all the subtests of
> the test yet.

In order to do that we would need to add support for profiles and refactor a bit
our type support code.

[Alicia Boya García]

(In reply to Michael Catanzaro from comment #10)
> Ummm yes.
> 
> Why are the expectations asserting that we don't accept these MIME types?

MSE spec requires types to not be allowed without a codec string. It also
forbids unmatched combinations (e.g. audio/webm with video codecs, or webm with
h264 video).

It also requires to support profiles in the codec strings (all those weird
looking numbers), which we currently do not support. See point 11 here:
https://developer.apple.com/library/content/documentation/NetworkingInternet/Conceptual/StreamingMediaGuide/FrequentlyAskedQuestions/FrequentlyAskedQuestions.html

[Manuel Rego Casasnovas]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Alberto Lopez Perez from comment #6)
> > What I'm unsure is about changing the expectation to contain FAIL lines. I
> > know some WebKit developers do that, but that always looked wrong to me.
> 
> We've basically decided to do that for WPT tests, yes, because that's what
> Youenn prefers and he's doing all of the work.

Yeah that's the agreed approach. The idea is that if you add also the FAIL messages
on the -expected result, you will notice when something changes.
If you just skip the test or mark a failure as a whole, you won't realize
when something changes from PASS to FAIL or the other way around.
So having the FAIL messages in the -expected file is useful as regression tests.

[Alicia Boya García]

Created attachment 323655 [details]
Patch

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Perhaps it would be helpful to split this patch in two: one patch to update the expectations file to trunk behaviour, and remove it from TestExpectations; and one patch on top of that to trim the spaces and (hopefully) only change FAIL -> PASS in the expectations file.

[Alicia Boya García]

Created attachment 324558 [details]
Patch

[Charlie Turner]

Comment on attachment 324558 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:847
> +        codec = codec.stripWhiteSpace();

I don't think we should be duplicating this logic, isn't the ContentType class in WebKit designed to handle this for us? See ContentType::parameter.

[Alicia Boya García]

Comment on attachment 324558 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:847
>> +        codec = codec.stripWhiteSpace();
> 
> I don't think we should be duplicating this logic, isn't the ContentType class in WebKit designed to handle this for us? See ContentType::parameter.

Good point: this function could receive a Vector<String> coming from ContentType::codecs() instead of parsing that parameter itself. I'll update the patch.

[Alicia Boya García]

Created attachment 324667 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 324667 [details]
Patch

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

Even when I did an r+ to the original patch, I think Charlie is right and you didn't address his comments in this patch. Maybe I misunderstood him but I just gave some guidance of how I think it shold be done.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:602
> +        if (!MediaPlayerPrivateGStreamerMSE::supportsCodecs({ originalMediaType })) {

If you keep the original signature + the overload, you don't need this.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:635
> +        if (!MediaPlayerPrivateGStreamerMSE::supportsCodecs({ structureName })) {

Ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:844
> +    for (String codec : codecs) {
> +        // Codecs might have whitespace around, e.g. when the string "vp8, vorbis" is split.
> +        codec = codec.stripWhiteSpace();

I think this won't be needed if you take const ContentType& here because that logic seems to be in that class.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:86
> -    static bool supportsCodecs(const String& codecs);
> +    static bool supportsCodecs(const Vector<String>& codecs);

I think this function signature should left unchanged and create an overloaded that takes a const ContentType&. The original function should create the ContentType from a String and pass it to the overloaded function.

[Alicia Boya García]

Comment on attachment 324667 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:602
>> +        if (!MediaPlayerPrivateGStreamerMSE::supportsCodecs({ originalMediaType })) {
> 
> If you keep the original signature + the overload, you don't need this.

Can a media type have two codecs somehow?

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:844
>> +        codec = codec.stripWhiteSpace();
> 
> I think this won't be needed if you take const ContentType& here because that logic seems to be in that class.

yep, I forgot to remove that.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:86
>> +    static bool supportsCodecs(const Vector<String>& codecs);
> 
> I think this function signature should left unchanged and create an overloaded that takes a const ContentType&. The original function should create the ContentType from a String and pass it to the overloaded function.

I'd rather not unless proven necessary. Overloads are confusing (at least compared to not having them) for people reading the code, so they should provide a clear benefit in exchange.

[Xabier Rodríguez Calvar]

(In reply to Alicia Boya García from comment #21)
> >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:602
> >> +        if (!MediaPlayerPrivateGStreamerMSE::supportsCodecs({ originalMediaType })) {
> > 
> > If you keep the original signature + the overload, you don't need this.
> 
> Can a media type have two codecs somehow?

I don't think so.

> >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:844
> >> +        codec = codec.stripWhiteSpace();
> > 
> > I think this won't be needed if you take const ContentType& here because that logic seems to be in that class.
> 
> yep, I forgot to remove that.

In this patch it was probably still necessary if you're only splitting by the ,. The result it would probably contain white spaces. Though this is irrelevant for changes we need to do.

> >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:86
> >> +    static bool supportsCodecs(const Vector<String>& codecs);
> > 
> > I think this function signature should left unchanged and create an overloaded that takes a const ContentType&. The original function should create the ContentType from a String and pass it to the overloaded function.
> 
> I'd rather not unless proven necessary. Overloads are confusing (at least
> compared to not having them) for people reading the code, so they should
> provide a clear benefit in exchange.

Considering what you mention in your first comment that applies to the second as well, maybe the approach should be creating a supportsContentType and a supportsCodec that checks only one codec. That way you could use the first in the last case and the second in the first two cases.

[Alicia Boya García]

(In reply to Xabier Rodríguez Calvar from comment #22)
> (In reply to Alicia Boya García from comment #21)
> > >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:844
> > >> +        codec = codec.stripWhiteSpace();
> > > 
> > > I think this won't be needed if you take const ContentType& here because that logic seems to be in that class.
> > 
> > yep, I forgot to remove that.
> 
> In this patch it was probably still necessary if you're only splitting by
> the ,. The result it would probably contain white spaces. Though this is
> irrelevant for changes we need to do.

No, it's not. ContentType::codecs() already parses them properly.


> > >> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:86
> > >> +    static bool supportsCodecs(const Vector<String>& codecs);
> > > 
> > > I think this function signature should left unchanged and create an overloaded that takes a const ContentType&. The original function should create the ContentType from a String and pass it to the overloaded function.
> > 
> > I'd rather not unless proven necessary. Overloads are confusing (at least
> > compared to not having them) for people reading the code, so they should
> > provide a clear benefit in exchange.
> 
> Considering what you mention in your first comment that applies to the
> second as well, maybe the approach should be creating a supportsContentType
> and a supportsCodec that checks only one codec. That way you could use the
> first in the last case and the second in the first two cases.

Okay, I'm fine with that.

[Alicia Boya García]

Created attachment 325016 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 325016 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:845
> +        codec = codec.substring(slashIndex+1);

slashIndex + 1

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:850
> +        bool codecMatchesPattern = !fnmatch(pattern.string().utf8().data(), codec.utf8().data(), 0);
> +        if (codecMatchesPattern)
> +            return true;

You don't need the variable, just if to the fnmatch and return true.

Btw, the name of the variable should be doesCodecMatchPattern according to https://webkit.org/code-style-guidelines/#names

[WebKit Commit Bot]

Comment on attachment 325016 [details]
Patch

Clearing flags on attachment: 325016

Committed r224107: <https://trac.webkit.org/changeset/224107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alicia Boya García]

Comment on attachment 325016 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:850
>> +            return true;
> 
> You don't need the variable, just if to the fnmatch and return true.
> 
> Btw, the name of the variable should be doesCodecMatchPattern according to https://webkit.org/code-style-guidelines/#names

I know the variable is syntactically redundant, but I introduced it on purpose to make the code easier to understand. 

Many people unfamiliar with `fnmatch()` would assume that `!fnmatch(...)` means *string does not match pattern* but turns out it is actually the opposite.

Introducing the variable I'm splitting the logic in two pieces that are easier to analyze: First it says "this variable tells if the codec matches the pattern, I know that by calling !fnmatch()" (when the reader gets surprised on the negation operator, they can consult `man fnmatch` like I did in the original code to confirm that it's actually intended). Then it says "if the codec matched (no matter how I computed it), return true".

I agree with the rename though.