Bug 192204

Created attachment 356099 [details] Patch

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

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

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

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

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

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

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

Created attachment 356159 [details] Patch

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?

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

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.

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.

Created attachment 356528 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 356528 [details]: media/remote-control-command-seek.html bug 192381 (authors: eric.carlson@apple.com and graouts@apple.com) The commit-queue is continuing to process your patch.

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: rego@igalia.com) webgl/2.0.0/conformance2/textures/canvas_sub_rectangle/tex-3d-rg16f-rg-float.html bug 192383 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch.

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

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

Comment on attachment 356528 [details] Patch for landing The failing tests are completely unrelated, let's try again.

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: roger_fong@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 356528 [details] Patch for landing Clearing flags on attachment: 356528 Committed r238892: <https://trac.webkit.org/changeset/238892>

All reviewed patches have been landed. Closing bug.