[webkit-reviews] review granted: [Bug 192204] [MSE][GStreamer] Remove the AppendPipeline state machine : [Attachment 356159] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 192204: [MSE][GStreamer] Remove the AppendPipeline state machine
https://bugs.webkit.org/show_bug.cgi?id=192204

Attachment 356159: Patch

https://bugs.webkit.org/attachment.cgi?id=356159&action=review




--- Comment #13 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 356159
  --> https://bugs.webkit.org/attachment.cgi?id=356159
Patch

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

There are some minor issues but once fixed, we can land.

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:212
>>> +		 g_critical("Unexpected appsink EOS in AppendPipeline");
>> 
>> ASSERT! (Is this the new "period at the end" to divert me from the real
deal? :D )
> 
> I prefer criticals because they are compiled and log errors in production,
whereas ASSERT() macros are not compiled.
> 
> I can accept GST_ERROR() followed by ASSERT_NOT_REACHED(), ... but
g_critical() achieves the same!

We had this discussions tons tons of times so I won't repeat myself ;)

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:283
>>> +	 ASSERT(!response);
>> 
>> What's the intention here, knowing if the ATQ is aborting or if the AP is
actually aborting. If the latter, I'd rather try some other check on the AP
itself. If the former, I'd try to actually check if the ATQ is aborting, not
just checking the answer.
> 
> enqueueTaskAndWait() returns an empty optional if and only if the
AsyncTaskQueue is aborting. Since the task handler indirectly but surely aborts
the AsyncTaskQueue(), there is no way the streaming thread would receive a
non-empty response, that's what I'm asserting. I could have not created a local
variable at all, but I wanted to assert.

This is not my point. If you want to return an empty response, fine, no
problem, but I think it is more robust to add a method do the ATQ to check if
it's aborting and then ASSERT on that. Feel free to leave the code to avoid the
WTFMove in the ATQ code, that's fine, but IMHO we should check the state with
the ATQ instead of the response.

>>>
LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasour
ce-invalid-codec-expected.txt:3
>>> +FAIL Test a WebM with an invalid codec results in an error. assert_true:
Media type not supported in this browser: isTypeSupported('video/webm;
codecs="vp8"') expected true got false
>> 
>> Wouldn't it be better to flag the whole test instead of creating this
expectations or is this never going to be supported?
> 
> The test checks for handling of invalid codecs in both MP4 files and WebM
files. Since Apple backends don't support WebM, they are not able to run the
WebM test, but that's no reason to skip the MP4 one. Therefore I think it makes
sense to use different output expectations instead.
> 
> Whether WebM is going to be supported in Apple backends eventually... I don't
know. If that would happen, many test expectations would have to be
rebaselined, of course, but that's something to be expected.

Ok.