Bug 165316 - [OWR] Unskip fast/mediastream/MediaStream-video-element-track-stop.html
Summary: [OWR] Unskip fast/mediastream/MediaStream-video-element-track-stop.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-02 11:00 PST by Alejandro G. Castro
Modified: 2017-01-09 03:04 PST (History)
8 users (show)

See Also:


Attachments
Patch (30.05 KB, patch)
2016-12-02 11:31 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (30.05 KB, patch)
2016-12-02 11:32 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (30.66 KB, patch)
2016-12-15 02:49 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (30.71 KB, patch)
2016-12-22 03:53 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (30.74 KB, patch)
2016-12-23 01:58 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2016-12-02 11:00:10 PST
We have to add proper support to the videoTracks interface and solve the modifycation of the ended attribute in the tracks.
Comment 1 Alejandro G. Castro 2016-12-02 11:31:24 PST
Created attachment 295968 [details]
Patch
Comment 2 Alejandro G. Castro 2016-12-02 11:32:34 PST
Created attachment 295969 [details]
Patch
Comment 3 Philippe Normand 2016-12-05 00:09:41 PST
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
Comment 4 Alejandro G. Castro 2016-12-13 10:23:21 PST
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.
Comment 5 Alejandro G. Castro 2016-12-15 02:49:21 PST
Created attachment 297188 [details]
Patch
Comment 6 WebKit Commit Bot 2016-12-15 05:16:09 PST
Comment on attachment 297188 [details]
Patch

Clearing flags on attachment: 297188

Committed r209860: <http://trac.webkit.org/changeset/209860>
Comment 7 WebKit Commit Bot 2016-12-15 05:16:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Csaba Osztrogonác 2016-12-17 01:30:42 PST
(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.
Comment 9 Alejandro G. Castro 2016-12-17 02:17:36 PST
(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.
Comment 10 Philippe Normand 2016-12-17 11:13:44 PST
This looks serious, the OWR player is selected for non-mediastream use-cases. I'll roll the patch out for now.
Comment 11 Philippe Normand 2016-12-17 11:16:10 PST
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 ()
Comment 12 Philippe Normand 2016-12-17 11:34:10 PST
Manual rollout in https://trac.webkit.org/changeset/209968 .
Comment 13 Alejandro G. Castro 2016-12-22 03:15:02 PST
(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.
Comment 14 Alejandro G. Castro 2016-12-22 03:53:55 PST
Created attachment 297651 [details]
Patch
Comment 15 Enrique Ocaña 2016-12-23 01:50:04 PST
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".
Comment 16 Alejandro G. Castro 2016-12-23 01:57:09 PST
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.
Comment 17 Alejandro G. Castro 2016-12-23 01:58:45 PST
Created attachment 297707 [details]
Patch
Comment 18 Philippe Normand 2016-12-25 02:35:22 PST
Comment on attachment 297707 [details]
Patch

Ok, hopefully this time it will stick :)
Comment 19 WebKit Commit Bot 2017-01-09 03:04:36 PST
Comment on attachment 297707 [details]
Patch

Clearing flags on attachment: 297707

Committed r210499: <http://trac.webkit.org/changeset/210499>
Comment 20 WebKit Commit Bot 2017-01-09 03:04:42 PST
All reviewed patches have been landed.  Closing bug.