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
The VideoTrackPrivate.h interface was updated only for the apple ports...
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
As of r287574, these tests now timeout rather than failing, but I think that's due to a change in the tests themselves.
(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 :)
Created attachment 448697 [details] Patch
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.
Created attachment 448759 [details] Patch
This seems to break mediastream tests, I'll try to check "soon" :(
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.
Created attachment 449250 [details] Patch
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].
<rdar://problem/87671934>
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.
(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.
Yacine, can you send a patch or should I take a look?
Maybe the player could unblock the tracks as part of its clean-up.
(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.
I'd advise to make use of an AbortableTaskQueue
(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.