Bug 165316 – [OWR] Unskip fast/mediastream/MediaStream-video-element-track-stop.html

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alejandro G. Castro]

We have to add proper support to the videoTracks interface and solve the modifycation of the ended attribute in the tracks.

[Alejandro G. Castro]

Created attachment 295968 [details]
Patch

[Alejandro G. Castro]

Created attachment 295969 [details]
Patch

[Philippe Normand]

Comment on attachment 295969 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:29
> +#include "MediaPlayerPrivateGStreamer.h"

Why is this needed?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:405
> +    /* auto mediaSource = OWR_MEDIA_SOURCE(realTimeMediaSource.mediaSource()); */

left-over? :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:412
> +        if (track.enabled())
> +            g_object_set(m_audioRenderer.get(), "disabled", false, nullptr);
> +        else
> +            g_object_set(m_audioRenderer.get(), "disabled", true, nullptr);

g_object_set(m_audioRenderer.get(), "disabled", !track.enabled(), nullptr);

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:421
> +            videoTrack->setSelected(false);

There's no need to call setSelected(true) in the other condition?

> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:359
> +        source = adoptRef(new RealtimeAudioSourceOwr(nullptr, id, type, name));
> +        name = "remote audio";

Shouldn't name be assigned before source?

> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:363
> +        source = adoptRef(new RealtimeVideoSourceOwr(nullptr, id, type, name));
> +        name = "remote video";

Ditto

[Alejandro G. Castro]

Thanks for the review :)

(In reply to comment #3)
> Comment on attachment 295969 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295969&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:29
> > +#include "MediaPlayerPrivateGStreamer.h"
> 
> Why is this needed?
> 

Left-over, I'll fix it.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:405
> > +    /* auto mediaSource = OWR_MEDIA_SOURCE(realTimeMediaSource.mediaSource()); */
> 
> left-over? :)
> 

Right.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:412
> > +        if (track.enabled())
> > +            g_object_set(m_audioRenderer.get(), "disabled", false, nullptr);
> > +        else
> > +            g_object_set(m_audioRenderer.get(), "disabled", true, nullptr);
> 
> g_object_set(m_audioRenderer.get(), "disabled", !track.enabled(), nullptr);
> 

Right.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:421
> > +            videoTrack->setSelected(false);
> 
> There's no need to call setSelected(true) in the other condition?
> 

Yep, the selection should happen when we load or unmute because it is enabled by default, but it is true we have to go back if we enable again. I'll review the logic again to make sure we have all the cases considering both specs involved.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:359
> > +        source = adoptRef(new RealtimeAudioSourceOwr(nullptr, id, type, name));
> > +        name = "remote audio";
> 
> Shouldn't name be assigned before source?
> 

Right.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:363
> > +        source = adoptRef(new RealtimeVideoSourceOwr(nullptr, id, type, name));
> > +        name = "remote video";
> 
> Ditto

Right.

[Alejandro G. Castro]

Created attachment 297188 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297188 [details]
Patch

Clearing flags on attachment: 297188

Committed r209860: <http://trac.webkit.org/changeset/209860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #6)
> Comment on attachment 297188 [details]
> Patch
> 
> Clearing flags on attachment: 297188
> 
> Committed r209860: <http://trac.webkit.org/changeset/209860>

It made 50+ tests crash on the GTK bot and the bot doeesn't run  any more tests after 50+ crashes. See build.webkit.org for details.

[Alejandro G. Castro]

(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 297188 [details]
> > Patch
> > 
> > Clearing flags on attachment: 297188
> > 
> > Committed r209860: <http://trac.webkit.org/changeset/209860>
> 
> It made 50+ tests crash on the GTK bot and the bot doeesn't run  any more
> tests after 50+ crashes. See build.webkit.org for details.

Thanks for the heads up, I'll check the issue.

[Philippe Normand]

This looks serious, the OWR player is selected for non-mediastream use-cases. I'll roll the patch out for now.

[Philippe Normand]

Thread 1 (Thread 0x2b4948768940 (LWP 20524)):
#0  0x00002b4939551f1b in _ZN7WebCore30MediaPlayerPrivateGStreamerOwr7setSizeERKNS_7IntSizeE () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#1  0x00002b49392f39f6 in _ZN7WebCore11RenderVideo12updatePlayerEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#2  0x00002b4938c8b0d1 in _ZN7WebCore16HTMLVideoElement14setDisplayModeENS_16HTMLMediaElement11DisplayModeE () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x00002b4938c8b225 in _ZN7WebCore16HTMLVideoElement18updateDisplayStateEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00002b4938c57dda in _ZN7WebCore16HTMLMediaElement12loadResourceERKNS_3URLERNS_11ContentTypeERKN3WTF6StringE () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x00002b4938c58961 in _ZN7WebCore16HTMLMediaElement19selectMediaResourceEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#6  0x00002b4938c5a0ba in _ZN7WebCore16HTMLMediaElement23pendingActionTimerFiredEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#7  0x00002b4938fb428f in _ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00002b493c3b74ba in _ZZN3WTF7RunLoop9TimerBaseC4ERS0_ENUlPvE_4_FUNES3_ () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#9  0x00002b493febeecd in g_main_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3122
#10 g_main_context_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3737
#11 0x00002b493febf268 in g_main_context_iterate () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3808
#12 0x00002b493febf582 in g_main_loop_run () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:4002
#13 0x00002b493c3b7860 in _ZN3WTF7RunLoop3runEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#14 0x00002b49385fcd42 in WebProcessMainUnix () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#15 0x00002b4942171b45 in __libc_start_main (main=0x400bf0 <main>, argc=2, argv=0x7ffe8dab93b8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffe8dab93a8) at libc-start.c:287
#16 0x0000000000400c45 in _start ()

[Philippe Normand]

Manual rollout in https://trac.webkit.org/changeset/209968 .

[Alejandro G. Castro]

(In reply to comment #12)
> Manual rollout in https://trac.webkit.org/changeset/209968 .

Thanks for the rollout, the engine selection code allows the elements to call API functions such as setSize even when it failed, until it gets the definitive engine, so we have to protect from that.

[Alejandro G. Castro]

Created attachment 297651 [details]
Patch

[Enrique Ocaña]

Comment on attachment 297651 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:492
> +        auto& realTimeMediaSource = static_cast<RealtimeMediaSourceOwr&>(m_videoTrack->source());

Have you checked if naturalSize() can be called before loading, during the player private selection process? In that case, m_videoTrack might be null and you could be dereferencing a null pointer. I suspect there might also be other cases where m_videoTrack could be null at this point.

> Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceOwr.h:76
> +    const RealtimeMediaSourceSettings& settings() const;

To my understanding, this should keep being "override".

[Alejandro G. Castro]

Thanks for the review.

(In reply to comment #15)
> Comment on attachment 297651 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297651&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:492
> > +        auto& realTimeMediaSource = static_cast<RealtimeMediaSourceOwr&>(m_videoTrack->source());
> 
> Have you checked if naturalSize() can be called before loading, during the
> player private selection process? In that case, m_videoTrack might be null
> and you could be dereferencing a null pointer. I suspect there might also be
> other cases where m_videoTrack could be null at this point.
> 

Good catch, yeah, m_videoTrack must be always protected.

> > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceOwr.h:76
> > +    const RealtimeMediaSourceSettings& settings() const;
> 
> To my understanding, this should keep being "override".

Yep, it is a typo merging in the last refactor.

Thanks again.

[Alejandro G. Castro]

Created attachment 297707 [details]
Patch

[Philippe Normand]

Comment on attachment 297707 [details]
Patch

Ok, hopefully this time it will stick :)

[WebKit Commit Bot]

Comment on attachment 297707 [details]
Patch

Clearing flags on attachment: 297707

Committed r210499: <http://trac.webkit.org/changeset/210499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.