Summary: | [GStreamer][MSE][EME] MSE specialization of Media Player Private for GStreamer | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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:55:22 PDT
Created attachment 290592 [details]
Patch
Created attachment 290594 [details]
Patch
Comment on attachment 290594 [details]
Patch
Wait until all the patches in 157314 are ready.
Comment on attachment 290594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290594&action=review These things can be fixed at landing time. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:35 > +#include "MIMETypeRegistry.h" This include is unsorted > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:49 > + You don't need this extra line > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:111 > + GST_DEBUG("%p", this); Meaningful message and also GST_TRACE > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:116 > + GST_DEBUG("destroying the player"); GST_TRACE > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:136 > + GST_DEBUG("Unsupported url: %s", urlString.utf8().data()); Shouldn't this be a GST_WARNING or GST_ERROR? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:162 > + return { }; I think it is clearer if we return MediaTime(); > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:197 > + GST_DEBUG("Seeking to %f failed", time); GST_WARNING? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:213 > + g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL); NULL -> nullptr > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:371 > + GST_DEBUG("Returning false"); > + return false; > + } > + > + // The samples will be enqueued in notifySeekNeedsData(). > + GST_DEBUG("Returning true"); Make these two message more meaningful, please. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:393 > + GST_DEBUG("[Seek] Seeking to %f failed", m_seekTime); GST_WARNING? > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:449 > + GST_DEBUG("Changing pipeline to PAUSED..."); > + bool ok = changePipelineState(GST_STATE_PAUSED); > + GST_DEBUG("Changed pipeline to PAUSED: %s", ok ? "Success" : "Error"); These two messages are bit redundant. If you really think you need them, probably first should be a _TRACE. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:527 > + m_resetPipeline = (state <= GST_STATE_READY); > + if (state <= GST_STATE_READY) These two lines are funny :) Changing them is your call. I don't mind. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:692 > + GST_DEBUG("m_mediaSourceClient is null, not doing anything"); My preference: not doing anything -> doing nothing > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785 > + for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator it = m_appendPipelinesMap.begin(); it != m_appendPipelinesMap.end(); ++it) { > > -> >> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:809 > + auto position = MediaPlayerPrivateGStreamer::currentMediaTime(); Type this, please. (In reply to comment #4) Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:35 > > +#include "MIMETypeRegistry.h" > > This include is unsorted Sorry, but the style checker thinks different (it uses case-sensitive string comparison): #include "MediaPlayer.h" #include "MIMETypeRegistry.h" #include "NotImplemented.h" ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:37: Alphabetical sorting problem. [build/include_order] [4] This explanation applies to all the other bugs where you've suggested a case-insensitive header sorting. Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:213 > > + g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL); > > NULL -> nullptr The g_object_set() API says (literally) that the last parameter must be NULL. Maybe in some strange platform NULL has a different value than nullptr. Anyway, I'll use nullptr. https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-set (In reply to comment #4) Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:449 > > + GST_DEBUG("Changing pipeline to PAUSED..."); > > + bool ok = changePipelineState(GST_STATE_PAUSED); > > + GST_DEBUG("Changed pipeline to PAUSED: %s", ok ? "Success" : "Error"); > > These two messages are bit redundant. If you really think you need them, > probably first should be a _TRACE. changePipelineState() does a lot of things internally, can potentially take some time to complete and even fail internally in the middle. It's interesting to have begin/end log messages to ease the interpretation of the gst log messages of other elements between the begin/end log messages. So ok, I'll change them to GST_TRACE(). Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785 > > + for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator it = m_appendPipelinesMap.begin(); it != m_appendPipelinesMap.end(); ++it) { I guess you'd like me to use "iterator" instead of "it" here. (In reply to comment #5) > Source/WebCore/platform/graphics/gstreamer/mse/ > MediaPlayerPrivateGStreamerMSE.cpp:213 > > > + g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL); > > > > NULL -> nullptr > > The g_object_set() API says (literally) that the last parameter must be > NULL. Maybe in some strange platform NULL has a different value than > nullptr. Anyway, I'll use nullptr. > > https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type. > html#g-object-set I agree with you but we have decided some time ago that we won't use NULL but nullptr instead. It is a matter of coding style then. Created attachment 291763 [details]
Patch
Created attachment 292256 [details]
Patch
Comment on attachment 292256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292256&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:109 > + , m_eosMarked(false) > + , m_eosPending(false) > + , m_gstSeekCompleted(true) > + , m_mseSeekCompleted(true) These can all be defined in the class declaration in the header. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:119 > + for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline>>::iterator iterator = m_appendPipelinesMap.begin(); iterator != m_appendPipelinesMap.end(); ++iterator) > + iterator->value->clearPlayerPrivate(); for (auto it : m_appendPipelinesMap) it.value->clearPlayerPrivate(); > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:150 > + String mediasourceUri = String::format("mediasource%s", url.utf8().data()); > + m_mediaSource = mediaSource; > + load(mediasourceUri); Just inline String::format() into the load() call. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:175 > + float current = currentMediaTime().toDouble(); You're clipping a double value to a float. Using currentMediaTime().toFloat() is the first step. The best solution (for a follow-up patch) would be to override seekMediaTime() and use MediaTime for this. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:351 > + gint64 startTime, endTime; > + > + if (rate > 0) { > + startTime = position; > + endTime = GST_CLOCK_TIME_NONE; > + } else { > + startTime = 0; > + endTime = position; > + } Alternatively, initialize startTime to 0 and endTime to `position`, and then override them in case `rate > 0`. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:491 > + m_source.clear(); m_source = nullptr; > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785 > + for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator iterator = m_appendPipelinesMap.begin(); iterator != m_appendPipelinesMap.end(); ++iterator) { for (auto it : m_appendPipelinesMap) > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:832 > + float result = durationMediaTime().toDouble(); Same here, clipping double to a float. Use toFloat() for now, but in the long-term we should switch to the MediaTime version of this method. Created attachment 292409 [details]
Patch
Created attachment 292894 [details]
Patch
Comment on attachment 292894 [details] Patch Clearing flags on attachment: 292894 Committed r207881: <http://trac.webkit.org/changeset/207881> All reviewed patches have been landed. Closing bug. |