Bug 162874

Created attachment 290503 [details] Patch

Created attachment 290504 [details] Patch

Comment on attachment 290504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290504&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:41 > + Remove extra line > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:93 > +static void appendPipelineElementMessageCallback(GstBus*, GstMessage* message, AppendPipeline* ap) > +{ > + ap->handleElementMessage(message); ap -> appendPipeline > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:330 > +#ifndef DEBUG_APPEND_PIPELINE_PADS It looks like this is !LOG_DISABLED now. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:357 > + totalAudio++; > + m_id = totalAudio; > + break; > + case Video: > + totalVideo++; > + m_id = totalVideo; > + break; > + case Text: > + totalText++; > + m_id = totalText; remove totalAudio++ and replace m_id = ++totalAudio; Applies also to video and text > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:362 > + GST_DEBUG("Trying to get id for a pipeline of Unknown/Invalid type"); > + ASSERT_NOT_REACHED(); If we are asserting here, this should be at least a GST_WARNING or a GST_ERROR > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:384 > + bool ok = false; Move this after the if (to the latest place where it can be declared. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:532 > + GstStructure* structure = gst_caps_get_structure(m_demuxerSrcPadCaps.get(), 0); > + const gchar* structureName = gst_structure_get_name(structure); > + GstVideoInfo info; > + bool sizeConfigured = false; structure and sizeConfigured should be moved to after setting m_streamType (latest place of declaration) structureName and info should be moved to inside if (!sizeConfigured) { > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:538 > + const gchar* originalMediaType = gst_structure_get_string(structure, "original-media-type"); This can be moved to just before it is used (and therefore we assert first, which is always useful) > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:606 > + // This means that we're right after a new track has appeared. Otherwise, it's a caps change inside the same track. > + bool previousCapsWereNull = !m_appsinkCaps; I think we can move this to just before the gst_caps_replace if. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:707 > + // Ignored. Operation completion will be managed by the Aborting->NotStarted transition. > + case Aborting: > + return; > + // Finish Ongoing and Sampling states. > + case Ongoing: > + setAppendState(DataStarve); > + break; > + case Sampling: > + setAppendState(LastSample); > + break; Move these comments to inside their corresponding case statements. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:739 > + GST_DEBUG("Unsupported or unknown stream type"); > + ASSERT_NOT_REACHED(); If we are asserting here, this should be at least a GST_WARNING or a GST_ERROR > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:865 > + GST_DEBUG("AppendPipeline has been disabled, ignoring this sample"); > + return GST_FLOW_ERROR; This also looks look like GST_WARNING or similar. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:868 > + m_newSampleLock.lock(); > + bool invalid = !m_playerPrivate || m_appendState == Invalid; > + m_newSampleLock.unlock(); > + > + // Even if we're disabled, it's important to pull the sample out anyway to > + // avoid deadlocks when changing to GST_STATE_NULL having a non empty appsink. > + GRefPtr<GstSample> sample = adoptGRef(gst_app_sink_pull_sample(GST_APP_SINK(appsink))); > + > + if (invalid) { > + GST_DEBUG("AppendPipeline has been disabled, ignoring this sample"); > + return GST_FLOW_ERROR; > + } > + > + LockHolder locker(m_newSampleLock); I think it would make sense to move getting the sample to the first place (after the assertion) and then we could apply the LockHolder to the whole function. This happens if gst_app_sink_pull_sample does generate deadlocks with m_newSampleLock, otherwise we could lock the whole function with the LockHolder. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:877 > + // an exceptional condition (entered in Invalid state, destructor, etc.) This sentence lacks a period at the end. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:893 > + // Only one Stream per demuxer is supported. Why is Stream capitalized? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:938 > +#if (ENABLE(ENCRYPTED_MEDIA) || ENABLE(ENCRYPTED_MEDIA_V2)) I think this ENCRYPTED_MEDIA is gone, you need to rebase. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:943 > + GRefPtr<GstPad> decryptorSrcPad = adoptGRef(gst_element_get_static_pad(m_decryptor.get(), "src")); This var is just used for the second link and can be declared just before that, right? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:968 > + // Only one Stream per demuxer is supported. Stream capitalized? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:978 > +#if (!(LOG_DISABLED || GST_DISABLE_GST_DEBUG)) We use GST_ log only so we can avoid checking for LOG_DISABLED > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1007 > + GST_DEBUG("(no data)"); > + GST_DEBUG("received all pending samples"); collapse these two messages: GST_DEBUG("no data, received all pending samples"); > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:32 > + Remove this extra line > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:110 > + // The demuxer has one src Stream only, so only one appsink is needed and linked to it. Why is Stream capitalized? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:125 > +#ifdef DEBUG_APPEND_PIPELINE_PADS It looks like this is !LOG_DISABLED now.

Comment on attachment 290504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290504&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:115 > + GST_DEBUG("%p", this); Please remove this or make the message significant :)

Comment on attachment 290504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290504&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:540 > + // Any previous decriptor should have been removed from the pipeline by disconnectFromAppSinkFromStreamingThread() decriptor -> decryptor and period at the end.

Created attachment 291756 [details] Patch

Attachment 291756 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:21: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 291757 [details] Patch

Comment on attachment 291757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291757&action=review Apart from these two nits, LGTM. As I wrote part of this code I guess somebody else will have to give it a look and r+. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:338 > + static gint totalAudio = 0; > + static gint totalVideo = 0; > + static gint totalText = 0; These variables can be moved to after the first if. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:529 > + // Any previous decriptor should have been removed from the pipeline by disconnectFromAppSinkFromStreamingThread() decriptor -> decryptor.

Comment on attachment 291757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291757&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:98 > +AppendPipeline::AppendPipeline(PassRefPtr<MediaSourceClientGStreamerMSE> mediaSourceClient, PassRefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate, MediaPlayerPrivateGStreamerMSE* playerPrivate) The parameter types should be adjusted to the nullness of the arguments being passed. RefPtr<> if the first two are possibly null, Ref<> if they're definitely not (but PassRefPtr<> should not be used). Same for playerPrivate -- if it's not null, then pass it as a reference, even if in the class you have to store it as a pointer because it's nulled out before the destruction. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:171 > + m_newSampleLock.lock(); > + setAppendState(Invalid); > + m_newSampleCondition.notifyOne(); > + m_newSampleLock.unlock(); Use a scope and a lock holder inside it. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:176 > + m_padAddRemoveLock.lock(); > + m_playerPrivate = nullptr; > + m_padAddRemoveCondition.notifyOne(); > + m_padAddRemoveLock.unlock(); Ditto. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:218 > + if (m_appsinkCaps) > + m_appsinkCaps = nullptr; Don't null-check it, just wreck it. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:221 > + if (m_demuxerSrcPadCaps) > + m_demuxerSrcPadCaps = nullptr; Ditto. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:236 > + m_newSampleLock.lock(); > + // Make sure that AppendPipeline won't process more data from now on and > + // instruct handleNewSample to abort itself from now on as well. > + setAppendState(Invalid); > + > + // Awake any pending handleNewSample operation in the streaming thread. > + m_newSampleCondition.notifyOne(); > + m_newSampleLock.unlock(); Scope + lock holder. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:241 > + m_padAddRemoveLock.lock(); > + m_playerPrivate = nullptr; > + m_padAddRemoveCondition.notifyOne(); > + m_padAddRemoveLock.unlock(); Scope + lock holder. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:338 >> + static gint totalText = 0; > > These variables can be moved to after the first if. Prepend them with 's_' to denote they are static. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:546 > + gst_structure_get_int(structure, "width", &width); > + if (gst_structure_get_int(structure, "height", &height)) { The return value for both of these should be tested. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:548 > + gint ratioNumerator = 1; > + gint ratioDenominator = 1; Use int instead of gint. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:568 > + const gchar* structureName = gst_structure_get_name(structure); const char*. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:643 > + m_newSampleLock.lock(); Scope + lock holder ... > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:654 > + // Ignore samples if we're not expecting them. Refuse processing if we're in Invalid state. > + if (m_appendState != Ongoing && m_appendState != Sampling) { > + GST_WARNING("Unexpected sample, appendState=%s", dumpAppendState(m_appendState)); > + // FIXME: Return ERROR and find a more robust way to detect that all the > + // data has been processed, so we don't need to resort to these hacks. > + // All in all, return OK, even if it's not the proper thing to do. We don't want to break the demuxer. > + m_flowReturn = GST_FLOW_OK; > + m_newSampleCondition.notifyOne(); > + return; > + } ... and here's the reason why you should prefer that -- you're not unlocking m_newSampleLock here. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:667 > + m_newSampleCondition.notifyOne(); > + return; And here. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:761 > + m_newSampleLock.lock(); > + m_newSampleCondition.notifyOne(); > + gst_element_set_state(m_pipeline.get(), GST_STATE_READY); > + gst_element_get_state(m_pipeline.get(), nullptr, nullptr, 0); > + m_newSampleLock.unlock(); Scope + lock holder. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:801 > + m_pendingBuffer.clear(); m_pendingBuffer = nullptr; > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:859 > + if (!(!m_playerPrivate || m_appendState == Invalid)) { This test should be simplified. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:881 > + GRefPtr<GstPad> sinkSinkPad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink")); sinkSinkPad? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:886 > + gint64 timeLength = 0; OK, I don't know at this point. The GLib typedefs are considered a mistake, that's clear. But I don't know to what degree we should be using them in this GStreamer-specific code. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:897 > + LockHolder locker(m_padAddRemoveLock); YES > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:922 > + // Must be done in the thread we were called from (usually streaming thread). > + bool isData = false; > + > + switch (m_streamType) { > + case WebCore::MediaSourceStreamTypeGStreamer::Audio: > + case WebCore::MediaSourceStreamTypeGStreamer::Video: > + case WebCore::MediaSourceStreamTypeGStreamer::Text: > + isData = true; > + break; > + default: > + break; > + } > + > + if (isData) { This could all just be folded into a larger if test, like `if (m_streamType == ... || m_streamType == ... || m_streamType == ...)`. Or at least initialize isData directly through the || expressions. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:957 > + LockHolder locker(m_padAddRemoveLock); YES > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1035 > + gsize bufferSize = gst_buffer_get_size(buffer); I guess it's fine. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:37 > + enum AppendState { Invalid, NotStarted, Ongoing, KeyNegotiation, DataStarve, Sampling, LastSample, Aborting }; This should be an enum class I think. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:91 > + RefPtr<MediaSourceClientGStreamerMSE> m_mediaSourceClient; > + RefPtr<SourceBufferPrivateGStreamer> m_sourceBufferPrivate; > + MediaPlayerPrivateGStreamerMSE* m_playerPrivate; Extending the comment on the constructor parameter types: - m_mediaSourceClient and m_sourceBufferPrivate are never expected to be null, so the constructor parameter types for these two should be Ref<>, as well as the types of these two members. - m_playerPrivate can be nulled out, so a pointer type for the member is fine. I don't think the argument passed to the constructor can be null, so I'd propose using a reference for that parameter type.

Created attachment 292248 [details] Patch

Created attachment 292401 [details] Patch

Comment on attachment 292401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292401&action=review LGTM. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:337 > + > + One newline is enough.

Created attachment 292738 [details] Patch

Created attachment 292888 [details] Patch

Comment on attachment 292888 [details] Patch Clearing flags on attachment: 292888 Committed r207875: <http://trac.webkit.org/changeset/207875>

All reviewed patches have been landed. Closing bug.