Bug 177355 – [GTK] Initial webm support in MSE

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

The attached patch adds initial experimental support for webm in WebKitGTK+ with
VP8, VP9, Vorbis and Opus formats, as long as suitable plugins are installed in
GStreamer.

[Alicia Boya García]

Created attachment 321540 [details]
Patch

[Zan Dobersek]

Comment on attachment 321540 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:143
> +    else
> +        ASSERT_NOT_REACHED();

This is the only concern I have. I suppose AppendPipeline wouldn't get created if the type doesn't match one of the two conditions?

[Alicia Boya García]

Indeed. The process is stopped much earlier in the call stack, in
MediaSource::addSourceBuffer()

[Xabier Rodríguez Calvar]

Comment on attachment 321540 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:143
>> +        ASSERT_NOT_REACHED();
> 
> This is the only concern I have. I suppose AppendPipeline wouldn't get created if the type doesn't match one of the two conditions?

A test here would be interesting if there is none. Feel free to submit it to the spec tests if there is none about this yet. If it is not accepted because of whatever, I think it still pays off to keep it as WebKit test. I guess the test would consist of creating a SourceBuffer with crap as type, try appending some allowed format and try to play it, expecting an error.

If the test fails already without calling this constructor, then we are good to go with the ASSERT_NOT_REACHED. If not, I agree with Žan here. I think the proper thing to do here would be to setting the AppendPipeline to Invalid state.

Then I see another problem coming and it is that apparently all state transitions from Invalid are "ok" and I think it shouldn't be like that. Only Invalid -> Invalid should be allowed. Everything else should be ok = false. I guess then we would have some ASSERTs in Debug mode that we'd have to fix in cases where somebody appends something to a pipeline in Invalid state. Am I missing anything?

[Alicia Boya García]

(In reply to Xabier Rodríguez Calvar from comment #4)
> A test here would be interesting if there is none.

There is already a test for that: imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer.html

If that did not work as expected it would crash but that's not the case.

[Xabier Rodríguez Calvar]

Comment on attachment 321540 [details]
Patch

(In reply to Alicia Boya García from comment #5)
> There is already a test for that:
> imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer.html
> 
> If that did not work as expected it would crash but that's not the case.

Good, you're landing then.

[WebKit Commit Bot]

Comment on attachment 321540 [details]
Patch

Clearing flags on attachment: 321540

Committed r222550: <http://trac.webkit.org/changeset/222550>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.