Bug 234084 – [GStreamer] tests media/track/audio-track-configuration.html and media/track/video-track-configuration.html fail

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Arcady Goldmints-Orlov]

These tests were added in r286754 and have been failing on both GTK and WPE since they were added. It appears that the configuration object exists but is not being populated with any information. The tests fail with the following diffs:

--- /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/media/track/audio-track-configuration-expected.txt
+++ /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/media/track/audio-track-configuration-actual.txt
@@ -3,9 +3,9 @@
 EVENT(canplay)
 EXPECTED (video.audioTracks.length > '0') OK
 EXPECTED (video.audioTracks[0].configuration != 'null') OK
-EXPECTED (video.audioTracks[0].configuration.codec == 'mp4a.40.2') OK
-EXPECTED (video.audioTracks[0].configuration.sampleRate == '44100') OK
-EXPECTED (video.audioTracks[0].configuration.numberOfChannels == '2') OK
-EXPECTED (video.audioTracks[0].configuration.bitrate >= '100000') OK
+EXPECTED (video.audioTracks[0].configuration.codec == 'mp4a.40.2'), OBSERVED '' FAIL
+EXPECTED (video.audioTracks[0].configuration.sampleRate == '44100'), OBSERVED '0' FAIL
+EXPECTED (video.audioTracks[0].configuration.numberOfChannels == '2'), OBSERVED '0' FAIL
+EXPECTED (video.audioTracks[0].configuration.bitrate >= '100000'), OBSERVED '0' FAIL
 END OF TEST

--- /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/media/track/video-track-configuration-expected.txt
+++ /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/media/track/video-track-configuration-actual.txt
@@ -3,27 +3,27 @@
 EVENT(canplay)
 EXPECTED (video.videoTracks.length == '1') OK
 EXPECTED (video.videoTracks[0].configuration != 'null') OK
-EXPECTED (video.videoTracks[0].configuration.codec == 'avc1.4D400D') OK
-EXPECTED (video.videoTracks[0].configuration.width == '320') OK
-EXPECTED (video.videoTracks[0].configuration.height == '240') OK
-EXPECTED (video.videoTracks[0].configuration.bitrate >= '240000') OK
-EXPECTED (video.videoTracks[0].configuration.framerate >= '30') OK
+EXPECTED (video.videoTracks[0].configuration.codec == 'avc1.4D400D'), OBSERVED '' FAIL
+EXPECTED (video.videoTracks[0].configuration.width == '320'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.height == '240'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.bitrate >= '240000'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.framerate >= '30'), OBSERVED '0' FAIL
 EXPECTED (video.videoTracks[0].configuration.colorSpace != 'null') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.matrix == 'bt470bg') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.primaries == 'smpte170m') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.transfer == 'bt709') OK
+EXPECTED (video.videoTracks[0].configuration.colorSpace.matrix == 'bt470bg'), OBSERVED 'null' FAIL
+EXPECTED (video.videoTracks[0].configuration.colorSpace.primaries == 'smpte170m'), OBSERVED 'null' FAIL
+EXPECTED (video.videoTracks[0].configuration.colorSpace.transfer == 'bt709'), OBSERVED 'null' FAIL
 RUN(video.src = "../content/test-hevc.mp4")
 EVENT(canplay)
 EXPECTED (video.videoTracks.length == '1') OK
 EXPECTED (video.videoTracks[0].configuration != 'null') OK
-EXPECTED (video.videoTracks[0].configuration.codec == 'hvc1.1.6.L93.B0') OK
-EXPECTED (video.videoTracks[0].configuration.width == '320') OK
-EXPECTED (video.videoTracks[0].configuration.height == '240') OK
-EXPECTED (video.videoTracks[0].configuration.bitrate >= '150000') OK
-EXPECTED (video.videoTracks[0].configuration.framerate >= '30') OK
+EXPECTED (video.videoTracks[0].configuration.codec == 'hvc1.1.6.L93.B0'), OBSERVED '' FAIL
+EXPECTED (video.videoTracks[0].configuration.width == '320'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.height == '240'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.bitrate >= '150000'), OBSERVED '0' FAIL
+EXPECTED (video.videoTracks[0].configuration.framerate >= '30'), OBSERVED '0' FAIL
 EXPECTED (video.videoTracks[0].configuration.colorSpace != 'null') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.matrix == 'bt709') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.primaries == 'bt709') OK
-EXPECTED (video.videoTracks[0].configuration.colorSpace.transfer == 'bt709') OK
+EXPECTED (video.videoTracks[0].configuration.colorSpace.matrix == 'bt709'), OBSERVED 'null' FAIL
+EXPECTED (video.videoTracks[0].configuration.colorSpace.primaries == 'bt709'), OBSERVED 'null' FAIL
+EXPECTED (video.videoTracks[0].configuration.colorSpace.transfer == 'bt709'), OBSERVED 'null' FAIL
 END OF TEST

[Philippe Normand]

The VideoTrackPrivate.h interface was updated only for the apple ports...

[Philippe Normand]

I've started a patch but:

1. Supporting playbin2 would be too much work
2. With playbin3 we would need the upcoming 1.20 release which provides gst_codec_utils_caps_get_mime_codec() but that's an incomplete implementation currently

[Arcady Goldmints-Orlov]

As of r287574, these tests now timeout rather than failing, but I think that's due to a change in the tests themselves.

[Philippe Normand]

(In reply to Arcady Goldmints-Orlov from comment #3)
> As of r287574, these tests now timeout rather than failing, but I think
> that's due to a change in the tests themselves.

Right, the tests are now full of promises which are a PITA to debug :)

[Philippe Normand]

Created attachment 448697 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 448697 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:59
> +    auto scopeExit = makeScopeExit([&] {
> +        setConfiguration(WTFMove(configuration));
> +    });

Why do you need this if I don't see any return?

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:59
> +    PlatformVideoTrackConfiguration configuration;
> +    auto scopeExit = makeScopeExit([&] {
> +        setConfiguration(WTFMove(configuration));
> +    });

This is bad.

First, regarding the life cycle of these objects, the compiler is going to destroy first the scopeExit and then the configuration object. But you're depending on something obscure.

Second and more dangerous. configuration belongs to the caller, when you create the lambda, you just copy a reference into the lambda but then inside the lambda, you move the object inside setConfiguration. configuration is going to be destructed when the caller dies first when the caller dies and then another time after setConfiguration finishes dealing with it. Very bad.

I think you can either avoid the scope exit or create the configuration object, get a pointer to it, move the object inside the lambda and operate with the pointer outside the lambda.

[Philippe Normand]

Created attachment 448759 [details]
Patch

[Philippe Normand]

This seems to break mediastream tests, I'll try to check "soon" :(

[Philippe Normand]

Comment on attachment 448759 [details]
Patch

This needs another iteration, the mediastreamsrc emits initial GstStreams with unfixated caps, so the TrackPrivates should handle that case and probably also watch for GstStream caps changes.

[Philippe Normand]

Created attachment 449250 [details]
Patch

[EWS]

Committed r288092 (246106@main): <https://commits.webkit.org/246106@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449250 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87671934>

[Yacine Bandou]

Created attachment 459538 [details]
threads callstacks

This patch causes a deadlock with the test "28. ChangeType.H264.VP9" in "https://ytlr-cert.appspot.com/2020/main.html"

See the threads callstacks.

The thread 50: We lock the gstpad and we send a signal "notify::caps" which is catched by "VideoTrackPrivateGStreamer::VideoTrackPrivateGStreamer" that calls "callOnMainThreadAndWait" to set the new configuration caps in the mainThread.

The thread 1 (mainThread): We destroy the "MediaPlayerPrivateGStreamer" and we set theGstPipeline to NULL that calls "gst_pad_set_active" but the pad is already locked by the thread50.

[Philippe Normand]

(In reply to Yacine Bandou from comment #13)
> Created attachment 459538 [details]
> threads callstacks
> 
> This patch causes a deadlock with the test "28. ChangeType.H264.VP9" in
> "https://ytlr-cert.appspot.com/2020/main.html"
> 
> See the threads callstacks.
> 
> The thread 50: We lock the gstpad and we send a signal "notify::caps" which
> is catched by "VideoTrackPrivateGStreamer::VideoTrackPrivateGStreamer" that
> calls "callOnMainThreadAndWait" to set the new configuration caps in the
> mainThread.
> 

We shouldn't do that when the notification comes for a pad being deactivated.

[Philippe Normand]

Yacine, can you send a patch or should I take a look?

[Philippe Normand]

Maybe the player could unblock the tracks as part of its clean-up.

[Yacine Bandou]

(In reply to Philippe Normand from comment #15)
> Yacine, can you send a patch or should I take a look?

I am looking for a fix, but not easy.

For me we should unblock the "VideoTrackPrivateGStreamer::updateConfigurationFromCaps"
callback whenwe disconnect the track in "VideoTrackPrivateGStreamer::disconnect".

As Olivier Blin suggested, we can use "callOnMainThread" with our own semaphore.

[Philippe Normand]

I'd advise to make use of an AbortableTaskQueue

[Yacine Bandou]

(In reply to Philippe Normand from comment #18)
> I'd advise to make use of an AbortableTaskQueue

Yes I think it is a good solution, I will try do it.