Summary: | [GStreamer][MSE] MediaSourceGStreamer refactoring | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrique Ocaña <eocanha> | ||||||||||||
Component: | Platform | Assignee: | Enrique Ocaña <eocanha> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | calvaris | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 157314 | ||||||||||||||
Attachments: |
|
Description
Enrique Ocaña
2016-10-04 04:28:45 PDT
Created attachment 290591 [details]
Patch
Comment on attachment 290591 [details]
Patch
Wait until all the patches in 157314 are ready.
Comment on attachment 290591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290591&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:49 > +#include <wtf/PassRefPtr.h> > +#include <wtf/glib/GRefPtr.h> These two lines are not alphabetically sorted. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:71 > + for (HashSet<SourceBufferPrivateGStreamer*>::iterator iterator = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); iterator != end; ++iterator) > + (*iterator)->clearMediaSource(); I am not sure if this would work, but you might want to try C++11 features like: for (sourceBuffer : m_sourceBuffers) sourceBuffer->clearMediaSource(); > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:76 > + RefPtr<SourceBufferPrivateGStreamer> buffer = SourceBufferPrivateGStreamer::create(this, m_client, contentType); Please, name this variable properly. This is not a buffer. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:71 > + virtual AddStatus addSourceBuffer(const ContentType&, RefPtr<SourceBufferPrivate>&); > + void removeSourceBuffer(SourceBufferPrivate*); > + > + virtual void durationChanged(); > + virtual void markEndOfStream(EndOfStreamStatus); > + virtual void unmarkEndOfStream(); > + > + virtual MediaPlayer::ReadyState readyState() const; > + virtual void setReadyState(MediaPlayer::ReadyState); > + > + virtual void waitForSeekCompleted(); > + virtual void seekCompleted(); These functions should not be tagged as virtual but as override. Comment on attachment 290591 [details]
Patch
I would like to have a deeper look to pointers here as I suspect we could use more refcounted
(In reply to comment #4) > I would like to have a deeper look to pointers here as I suspect we could > use more refcounted I've converted m_sourceBuffers to be a HashSet<RefPtr<SourceBufferPrivateGStreamer>>, and m_mediaSource is now a RefPtr<MediaSourcePrivateClient>. I'll be submitting an updated patch soon. For the case of m_activeSourceBuffers and m_playerPrivate I think it's fine to use raw pointers. In the first case, there won't be any active source buffer which is not in already in the set of source buffers. This is alse the strategy followed by Apple's MediaPlayerPrivateMediaSourceAVFObjC. For the case of m_playerPrivate, the thing is that none of MediaPlayerPrivateGStreamerMSE or its ancestors inherit from RefCounted. MediaPlayerPrivateInterface is just not meant to be RefCounted, because MediaPlayer holds an std::unique_ptr<MediaPlayerPrivateInterface>. Therefore, all the reference holding elsewhere must be done using raw pointers. Created attachment 291762 [details]
Patch
Comment on attachment 291762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291762&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82 > + RefPtr<MediaSourcePrivateClient> m_mediaSource; Are we sure we don't have a circular reference here? (In reply to comment #7) > > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82 > > + RefPtr<MediaSourcePrivateClient> m_mediaSource; > Are we sure we don't have a circular reference here? Yes, I've checked that the destructors of both MediaSourcePrivateClient and MediaSourceGStreamer are called at runtime. It wouldn't be so if we had a circular reference. Created attachment 292254 [details]
Patch
Comment on attachment 292254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292254&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:56 > + ASSERT(mediaSource); > + ASSERT(playerPrivate); Use references for the parameters if you're expecting non-null values, not pointers. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:58 > + RefPtr<MediaSourceGStreamer> mediaSourcePrivate = adoptRef(new MediaSourceGStreamer(*mediaSource, *playerPrivate)); Use a Ref<> and adoptRef(*new ...) combo. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:59 > + mediaSourcePrivate->m_playerPrivate = playerPrivate; Do this in the constructor. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:60 > + mediaSource->setPrivateAndOpen(mediaSourcePrivate.releaseNonNull()); You'd move mediaSourcePrivate (now a Ref<>) in here, but since the m_playerPrivate assignment would be moved into the constructor, you might as well inline the adoptRef(*new MediaSourceGStreamer) call into this setPrivateAndOpen() call. So together with using references for parameter types of this open() method, you should end up with a one-liner. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:67 > + m_client = MediaSourceClientGStreamerMSE::create(playerPrivate); m_client can be assigned in the initialization list of this constructor. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:72 > + for (RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate : m_sourceBuffers) for (auto& sourceBufferPrivate : m_sourceBuffers) > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:86 > + RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivateGStreamer = reinterpret_cast<SourceBufferPrivateGStreamer*>(sourceBufferPrivate); This should be a static_cast, like in the method above. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82 > + RefPtr<MediaSourcePrivateClient> m_mediaSource; This can be a Ref<>. > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:83 > + MediaPlayerPrivateGStreamerMSE* m_playerPrivate; This can be a reference. Created attachment 292408 [details]
Patch
Comment on attachment 292408 [details]
Patch
LGTM.
Created attachment 292893 [details]
Patch
Comment on attachment 292893 [details] Patch Clearing flags on attachment: 292893 Committed r207880: <http://trac.webkit.org/changeset/207880> All reviewed patches have been landed. Closing bug. |