Bug 210840 – [GStreamer][MediaStream] fast/mediastream/play-newly-added-audio-track.html is failing since added in r260380

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lauro Moura]

It is also flaky crashing with a similar backtrace to bug210498.

Diff:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/mediastream/play-newly-added-audio-track-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/mediastream/play-newly-added-audio-track-actual.txt
@@ -1,5 +1,5 @@
 
 
-PASS Add an audio track while playing video 
-PASS Add an audio track while playing audio 
+FAIL Add an audio track while playing video promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('track') to Internals.shouldAudioTrackPlay must be an instance of AudioTrack"
+FAIL Add an audio track while playing audio promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('track') to Internals.shouldAudioTrackPlay must be an instance of AudioTrack"

[Víctor M. Jáquez L.]

This is a new test from r260380

[Lauro Moura]

Flaky timing out since r273754 (first timeout).

[Philippe Normand]

Created attachment 426203 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 426203 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1660
> +        webkitMediaStreamSrcConfigureAudioTracks(WEBKIT_MEDIA_STREAM_SRC(m_source.get()), volume(), isMuted(), !m_isPaused);

Can't we _CAST here?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:125
> +    InternalSource(GstElement* parent, MediaStreamTrackPrivate& track, String padName)

You can take a const String& here and let assignment to the attribute do the copy.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:482
> +        GST_OBJECT_UNLOCK(self);
> +        return;
> +    }
> +
> +    auto upstreamId = priv->stream ? priv->stream->id() : createCanonicalUUIDString();
> +    priv->streamCollection = adoptGRef(gst_stream_collection_new(upstreamId.ascii().data()));
> +    for (auto& track : priv->tracks) {
> +        if (!track->isActive())
> +            continue;
> +        gst_stream_collection_add_stream(priv->streamCollection.get(), webkitMediaStreamNew(track.get()));
> +    }
> +
> +    GST_OBJECT_UNLOCK(self);

For these cases I would like to use ScopeExit on Scope.h. That way you could create the ScopeExit to run the GST_OBJECT_UNLOCK and forget to UNLOCK in the first place. In case you need to add more early returns, you're covered.

The problem is that ScopeExit does not have a runEarlyAndRelease method yet that you would need for the second UNLOCK. If you feel like adding it, you have my blessing. The patter would be similar to the unlockEarly of the LockHolder.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:523
> +    const String& padName;

ProbeData is most probably not going to outlive the InternalSource but it would be safer to not have const String& and just a String here. We just make a copy of the String object, the StringImpl does the refcounting and everybody is happier.

[Philippe Normand]

Comment on attachment 426203 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1660
>> +        webkitMediaStreamSrcConfigureAudioTracks(WEBKIT_MEDIA_STREAM_SRC(m_source.get()), volume(), isMuted(), !m_isPaused);
> 
> Can't we _CAST here?

We had no such variant, until now. I'm adding it.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:482
>> +    GST_OBJECT_UNLOCK(self);
> 
> For these cases I would like to use ScopeExit on Scope.h. That way you could create the ScopeExit to run the GST_OBJECT_UNLOCK and forget to UNLOCK in the first place. In case you need to add more early returns, you're covered.
> 
> The problem is that ScopeExit does not have a runEarlyAndRelease method yet that you would need for the second UNLOCK. If you feel like adding it, you have my blessing. The patter would be similar to the unlockEarly of the LockHolder.

I agree improving ScopeExit would be nice but for this code it wouldn't be useful anyway. The runEarlyAndRelease() would need to be called before gst_element_post_message() to avoid a deadlock, so that defeats the purpose of using ScopeExit here :)

[Philippe Normand]

Created attachment 426328 [details]
Patch

[Philippe Normand]

Committed r276197 (236679@main): <https://commits.webkit.org/236679@main>

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #5)
> > The problem is that ScopeExit does not have a runEarlyAndRelease method yet that you would need for the second UNLOCK. If you feel like adding it, you have my blessing. The patter would be similar to the unlockEarly of the LockHolder.
> 
> I agree improving ScopeExit would be nice but for this code it wouldn't be
> useful anyway. The runEarlyAndRelease() would need to be called before
> gst_element_post_message() to avoid a deadlock, so that defeats the purpose
> of using ScopeExit here :)

It's true that it defeats the purpose "in this case", but GStreamer codes is used to having structure of early return that force you to release resources before returning, either locks, buffers or objects. This is an easy case with one lock, release close to the end and one early return. If I prefer the ScopeExit is because it prevents errors in case you add another early return if you happen to forget to release the lock. Am I overprotective, won't deny it either :)