Bug 192204 – [MSE][GStreamer] Remove the AppendPipeline state machine

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

[Alicia Boya García]

This patch tries to reduce the complexity of the AppendPipeline by
removing the appendState state machine and cleaning all the
conditional code around it that is not necessary anymore.

For the most part the behavior is the same, but some edge cases have
been improved in the process:

Demuxing errors now result in the append being flagged as
ParsingFailed and the error being propagated to the application. This
fixes media/media-source/media-source-error-crash.html (or at least
gets it up to date with cross platform expectations).

AbortableTaskQueue now allows the task handler to perform an abort
safely. This is used in the GstBus error message sync handler, since
it needs to ask the MainThread to raise a parse error, which will in
turn abort. An API test has been added for this new functionality.
Also, code has been added to the API tests to ensure the correct
destruction of the response object, especially in this case.

The code handling invalid track codecs has been made clearer by also
explicitly raising a parse error, but it should not expose behavior
differences for the application. A test has been added for this
behavior: web-platform-tests/media-source/mediasource-invalid-codec.html

The reporting of EOS events have been made more rigorous. EOS is only
expected after a demuxing error, otherwise it's a g_critical.

AppendPipeline::abort() has been renamed to
AppendPipeline::resetParserState() to honor the fact that it's not
only called when the user calls abort() and match better the names
used in the spec.

[Alicia Boya García]

Created attachment 356099 [details]
Patch

[Alicia Boya García]

Comment on attachment 356099 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:521
> +    // TODO: Implement a flush event-based resetParserState() implementation would allow the initialization segment to
> +    // survive, in accordance with the spec.

This will be possible with the famous qtdemux MSE-style flush patch, but I preferred to leave that aside for a separate patch. https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/467

[EWS Watchlist]

Comment on attachment 356099 [details]
Patch

Attachment 356099 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10207071

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html

[EWS Watchlist]

Created attachment 356104 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356099 [details]
Patch

Attachment 356099 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10207276

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html

[EWS Watchlist]

Created attachment 356109 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356099 [details]
Patch

Attachment 356099 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10207677

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html

[EWS Watchlist]

Created attachment 356116 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Alicia Boya García]

Created attachment 356159 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 356159 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:83
> +// Wrapper for gst_element_set_state() that emits a critical if the state change fails or is not synchronous.
> +static void assertedElementSetState(GstElement* element, GstState desiredState)

Why do we need this function? Can you please elabortate on why things need to be sync?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:88
> +    gst_element_set_state(element, desiredState);

Not strong opinion about it, but you could read the return value and ASSERT on it as well (Yes, you can guess my next comment)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:95
> +        g_critical("AppendPipeline state change failed. %" GST_PTR_FORMAT " %d -> %d (expected %d)", element,
> +            static_cast<int>(oldState), static_cast<int>(newState), static_cast<int>(desiredState));

GST_ERROR or WARNING at your choice and ASSERT_NOT_REACHED, no g_critical.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:154
> +    gst_base_sink_set_async_enabled(GST_BASE_SINK(m_appsink.get()), FALSE); // No prerolls, no async state changes

This explains that you want to assert in the code above, but I am still interested in why.

Period at the end! (I'll continue reviewing, you didn't stop me yet :D ).

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:155
> +    gst_base_sink_set_drop_out_of_segment(GST_BASE_SINK(m_appsink.get()), FALSE);

?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:210
> +        // basesrc will emit a EOS after it has received a GST_FLOW_ERROR. That's the only case we are expecting.

aN EOS

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:212
> +        if (!appendPipeline->m_errorReceived)
> +            g_critical("Unexpected appsink EOS in AppendPipeline");

ASSERT! (Is this the new "period at the end" to divert me from the real deal? :D )

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:219
> +    assertedElementSetState(m_pipeline.get(), GST_STATE_PLAYING);

Why to PLAYING now?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:283
> +    // The streaming thread has now been unblocked because we are aborting in the main thread.
> +    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.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:294
> +    GST_TRACE("Buffer entered appsrcEndOfAppendCheckerProbe: %" GST_PTR_FORMAT, buffer);

Above you trace with _OBJECT(m_pipeline().get(), why not here? Might be interesting to be more consistent?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:316
> +    m_playerPrivate->handleSyncMessage(message);

You sure we have a player private here? Might be interesting to ASSERT? I see this as a trend in the rest of the code as well.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:446
> +        GST_DEBUG("Detected sample (%f) beyond the duration (%f), discarding", mediaSample->presentationTime().toFloat(), duration.toFloat());

I don't have a strong opinion on this but it might be interesting to use MediaTime::toString method here, instead of just a plain float.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:520
> +    // TODO: Implement a flush event-based resetParserState() implementation would allow the initialization segment to

There are no TODOs, only FIXMEs.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:526
> +    // Unlock the streaming thread

.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:529
> +    // Reset the state of all elements in the pipeline

.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:105
> +    // Used only for asserting EOS events are only caused by demuxing errors.

A native speaker might un-correct this but I think that it should be: Used only to assert that ...

> LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-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?

[Michael Catanzaro]

(In reply to Xabier Rodríguez Calvar from comment #10)
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:105
> > +    // Used only for asserting EOS events are only caused by demuxing errors.
> 
> A native speaker might un-correct this but I think that it should be: Used
> only to assert that ...

Either way is good!

[Alicia Boya García]

Comment on attachment 356159 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:83
>> +static void assertedElementSetState(GstElement* element, GstState desiredState)
> 
> Why do we need this function? Can you please elabortate on why things need to be sync?

Functionally speaking, we don't need it: we could just call gst_element_set_state(). What this function adds is error checking, so that should a future bug make a state change fail, it fails in this call, not later.

Asynchronous state changes occur when we ask PAUSED -> PLAYING and the pipeline waits for preroll. AppendPipeline is about demuxing, not playback, so we don't want any prerolling; therefore we disable it in the sinks and expect state transitions to be instantaneous (as they are in absence of prerolls).

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:88
>> +    gst_element_set_state(element, desiredState);
> 
> Not strong opinion about it, but you could read the return value and ASSERT on it as well (Yes, you can guess my next comment)

OK.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:154
>> +    gst_base_sink_set_async_enabled(GST_BASE_SINK(m_appsink.get()), FALSE); // No prerolls, no async state changes
> 
> This explains that you want to assert in the code above, but I am still interested in why.
> 
> Period at the end! (I'll continue reviewing, you didn't stop me yet :D ).

This disables prerolling.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:155
>> +    gst_base_sink_set_drop_out_of_segment(GST_BASE_SINK(m_appsink.get()), FALSE);
> 
> ?

This disables GstSegment clipping in the sink. It's not usually a problem, and may be a bit unrelated, but I just wanted to reduce the magic GStreamer does in the shadows.

>> 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!

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:219
>> +    assertedElementSetState(m_pipeline.get(), GST_STATE_PLAYING);
> 
> Why to PLAYING now?

The real question would be why to READY before :)

By disabling prerolls I was able to get rid of the complicated element state switching logic that was in the AppendPipeline state machine. Now the AppendPipeline starts as PLAYING and it only changes state when it's destroyed (to NULL) or momentarily to READY in resetParserState() when we want to reset it.

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:316
>> +    m_playerPrivate->handleSyncMessage(message);
> 
> You sure we have a player private here? Might be interesting to ASSERT? I see this as a trend in the rest of the code as well.

Yes. m_playerPrivate is initialized in the AppendPipeline constructor and, since I refactor deinitialization in https://bugs.webkit.org/show_bug.cgi?id=191759, never changed. There is no need for checking it anymore.

>> LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-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.

[Xabier Rodríguez Calvar]

Comment on attachment 356159 [details]
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/mediasource-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.

[Alicia Boya García]

Created attachment 356528 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356528 [details]:

media/remote-control-command-seek.html bug 192381 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356528 [details]:

imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-percentage-rows-indefinite-height-001.html bug 192382 (author: [e-mail])
webgl/2.0.0/conformance2/textures/canvas_sub_rectangle/tex-3d-rg16f-rg-float.html bug 192383 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356528 [details]
Patch for landing

Rejecting attachment 356528 [details] from commit-queue.

New failing tests:
workers/bomb.html
Full output: https://webkit-queues.webkit.org/results/10269028

[WebKit Commit Bot]

Created attachment 356541 [details]
Archive of layout-test-results from webkit-cq-03 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Alicia Boya García]

Comment on attachment 356528 [details]
Patch for landing

The failing tests are completely unrelated, let's try again.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356528 [details]:

webgl/1.0.2/conformance/more/functions/bindFramebufferLeaveNonZero.html bug 192399 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356528 [details]
Patch for landing

Clearing flags on attachment: 356528

Committed r238892: <https://trac.webkit.org/changeset/238892>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.