Bug 162896

Created attachment 290589 [details] Patch

Created attachment 290593 [details] Patch

Comment on attachment 290593 [details] Patch Wait until all the patches in 157314 are ready.

Comment on attachment 290593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290593&action=review These things can be fixed at landing time > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:47 > + m_playerPrivate = playerPrivate; Can't you do this assigment in the constructor? > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:63 > + GST_DEBUG("this=%p sourceBuffer=%p appendPipeline=%p", this, sourceBufferPrivate.get(), appendPipeline.get()); You might want to add a more meaningful message and probably GST_TRACE instead. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:66 > + ASSERT(m_playerPrivate->m_playbackPipeline); If we are going to assert this, let's do it sooner, after the first return. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:141 > + ASSERT(m_playerPrivate->m_playbackPipeline); Let's assert this sooner, after the first return. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:177 > + return GRefPtr<WebKitMediaSrc>(nullptr); > + > + WebKitMediaSrc* source = WEBKIT_MEDIA_SRC(m_playerPrivate->m_source.get()); > + > + ASSERT(WEBKIT_IS_MEDIA_SRC(source)); > + > + return GRefPtr<WebKitMediaSrc>(source); Let's do the implicit constructor do its job if possible.

Created attachment 291760 [details] Patch

Comment on attachment 291760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291760&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:32 > +PassRefPtr<MediaSourceClientGStreamerMSE> MediaSourceClientGStreamerMSE::create(MediaPlayerPrivateGStreamerMSE* playerPrivate) This should return Ref<>. And probably take playerPrivate via a reference, assuming it is not null. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:42 > +MediaSourceClientGStreamerMSE::MediaSourceClientGStreamerMSE(MediaPlayerPrivateGStreamerMSE* playerPrivate) Ditto on playerPrivate being a reference. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:70 > +MediaTime MediaSourceClientGStreamerMSE::duration() This method can return const MediaTime&. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:90 > +void MediaSourceClientGStreamerMSE::abort(PassRefPtr<SourceBufferPrivateGStreamer> prpSourceBufferPrivate) This shouldn't be receiving a PassRefPtr<>. Supposing the argument is not null, it should be a Ref<>. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:99 > + RefPtr<SourceBufferPrivateGStreamer> protectedPrpSourceBufferPrivate = prpSourceBufferPrivate; With a Ref<> or even a RefPtr<> argument, you now don't need this protector. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:100 > + RefPtr<AppendPipeline> appendPipeline = m_playerPrivate->m_appendPipelinesMap.get(protectedPrpSourceBufferPrivate); There should be an assert that ensures the SourceBufferPrivateGStreamer object is in the map. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:104 > +bool MediaSourceClientGStreamerMSE::append(PassRefPtr<SourceBufferPrivateGStreamer> prpSourceBufferPrivate, const unsigned char* data, unsigned length) Ditto on the PassRefPtr argument. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:113 > + RefPtr<SourceBufferPrivateGStreamer> protectedPrpSourceBufferPrivate = prpSourceBufferPrivate; Ditto on not needing a protector anymore. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:118 > + return GST_FLOW_OK == appendPipeline->pushNewBuffer(buffer); Comparing a return value to a constant is easier to read. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:125 > + m_playerPrivate->markEndOfStream(status); This method doesn't check for a null m_playerPrivate. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:137 > + RefPtr<AppendPipeline> appendPipeline = m_playerPrivate->m_appendPipelinesMap.get(sourceBufferPrivate); An assert would be welcome to make sure a pointer to the object is present in the map. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:152 > + if (!m_playerPrivate) > + return; > + > + m_playerPrivate->m_playbackPipeline->flushAndEnqueueNonDisplayingSamples(samples); These null-checked single calls are better just compressed into two lines, without an early return: if (m_playerPrivate) m_playerPrivate->whatever(). > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:36 > +class ContentType; > +class SourceBufferPrivateGStreamer; > +class MediaPlayerPrivateGStreamerMSE; > +class MediaSample; Alphabetic order. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:38 > +class MediaSourceClientGStreamerMSE: public RefCounted<MediaSourceClientGStreamerMSE> { A missing space before the colon. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:64 > + // Would better be a RefPtr, but the playerprivate is a unique_ptr, so we can't mess with references here. In > + // exchange, the playerprivate must notify us when it's being destroyed, so we can clear our pointer. This comment can be removed, it's not used in other patches.

Comment on attachment 291760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291760&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:28 > +#if ENABLE(VIDEO) && USE(GSTREAMER) && ENABLE(MEDIA_SOURCE) && ENABLE(VIDEO_TRACK) Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to guard HTML5 video subtitles support. I don't think it has anything to do with MSE.

(In reply to comment #7) > Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to > guard HTML5 video subtitles support. I don't think it has anything to do > with MSE. The ENABLE(VIDEO_TRACK) is used to guard code which needs "track" support (whatever is related to TrackPrivateBase). This includes audio, video and text tracks (subtitles). There's code (eg: the regular MediaPlayerPrivateGStreamer) which in theory can work with track support disabled. MSE, on the contrary, is strongly coupled with the track support[1], so it makes no sense to enable one without the other. That's why all the code related to MSE is also guarded with ENABLE(VIDEO_TRACK). [1] https://w3c.github.io/media-source/#video-track-extensions

Created attachment 292252 [details] Patch

(In reply to comment #8) > (In reply to comment #7) > > > Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to > > guard HTML5 video subtitles support. I don't think it has anything to do > > with MSE. > > The ENABLE(VIDEO_TRACK) is used to guard code which needs "track" support > (whatever is related to TrackPrivateBase). This includes audio, video and > text tracks (subtitles). > > There's code (eg: the regular MediaPlayerPrivateGStreamer) which in theory > can work with track support disabled. MSE, on the contrary, is strongly > coupled with the track support[1], so it makes no sense to enable one > without the other. That's why all the code related to MSE is also guarded > with ENABLE(VIDEO_TRACK). > > [1] https://w3c.github.io/media-source/#video-track-extensions Here's what's guarded with VIDEO_TRACK in Modules/mediasource/: - three attributes on the SourceBuffer interface, - extensions to the AudioTrack, TextTrack and VideoTrack interfaces. So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK disabled, those attributes and interface extensions would disappear, leaving the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot would vanish.

(In reply to comment #10) > Here's what's guarded with VIDEO_TRACK in Modules/mediasource/: > - three attributes on the SourceBuffer interface, > - extensions to the AudioTrack, TextTrack and VideoTrack interfaces. > > So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK > disabled, those attributes and interface extensions would disappear, leaving > the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot > would vanish. No. If a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK disabled, it will fail. To start with, because the methods guarded in SourceBuffer.h aren't guarded in SourceBuffer.cpp. SourceBuffer::m_{audio,video,text}Tracks are used all over the place unguarded. Also, SourceBufferPrivateClient (SourceBuffer's superclass) manages the concept of InitializationSegment, which holds {Audio,Video,Text}TrackInformation with references to {Audio,Video,Text}TrackPrivate. Everything unguarded. Therefore, my conclusion is that MEDIA_SOURCE and VIDEO_TRACK are indivisibly related. That's why I ask for both in all our private GStreamer implementation. Does it really make sense to "fix the world" just to have ENABLE(VIDEO_TRACK) nicely splitted in the GStreamer private implementation?

(In reply to comment #11) > (In reply to comment #10) > > > Here's what's guarded with VIDEO_TRACK in Modules/mediasource/: > > - three attributes on the SourceBuffer interface, > > - extensions to the AudioTrack, TextTrack and VideoTrack interfaces. > > > > So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK > > disabled, those attributes and interface extensions would disappear, leaving > > the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot > > would vanish. > > No. If a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK > disabled, it will fail. To start with, because the methods guarded in > SourceBuffer.h aren't guarded in SourceBuffer.cpp. > SourceBuffer::m_{audio,video,text}Tracks are used all over the place > unguarded. > > Also, SourceBufferPrivateClient (SourceBuffer's superclass) manages the > concept of InitializationSegment, which holds > {Audio,Video,Text}TrackInformation with references to > {Audio,Video,Text}TrackPrivate. Everything unguarded. > > Therefore, my conclusion is that MEDIA_SOURCE and VIDEO_TRACK are > indivisibly related. That's why I ask for both in all our private GStreamer > implementation. > > Does it really make sense to "fix the world" just to have > ENABLE(VIDEO_TRACK) nicely splitted in the GStreamer private implementation? 'Indivisibly related'? 'Fix the world'? It's a matter of a few patches to fix this. OptionsGTK.cmake enables VIDEO_TRACK option but keeps it private. IMO that should be a good enough guarantee that it's always enabled as long as ENABLE_VIDEO is as well. MEDIA_SOURCE will depend on ENABLE_VIDEO as well. VIDEO_TRACK should probably be taken as enabled by default, as long as video is supported.

(In reply to comment #12) > 'Indivisibly related'? 'Fix the world'? It's a matter of a few patches to > fix this. > To clarify -- it's trivially fixable, but don't bother with it. There's no one disabling VIDEO_TRACK but keeping MEDIA_SOURCE enabled, obviously. Just assume that if VIDEO is enabled, so is VIDEO_TRACK, and MEDIA_SOURCE relies on VIDEO, hence you don't have to support a configuration where VIDEO_TRACK is disabled but MEDIA_SOURCE is enabled. That's why you shouldn't bother with VIDEO_TRACK guards in platform/graphics/gstreamer/mse/.

Created attachment 292406 [details] Patch

Comment on attachment 292406 [details] Patch Attachment 292406 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2340538 New failing tests: svg/wicd/test-rightsizing-b.xhtml

Created attachment 292419 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Created attachment 292779 [details] Patch

Created attachment 292891 [details] Patch

Comment on attachment 292891 [details] Patch Clearing flags on attachment: 292891 Committed r207878: <http://trac.webkit.org/changeset/207878>

All reviewed patches have been landed. Closing bug.