[webkit-reviews] review denied: [Bug 199719] [MSE][GStreamer] WebKitMediaSrc rework : [Attachment 374830] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 25 07:08:33 PDT 2019


Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 199719: [MSE][GStreamer] WebKitMediaSrc rework
https://bugs.webkit.org/show_bug.cgi?id=199719

Attachment 374830: Patch

https://bugs.webkit.org/attachment.cgi?id=374830&action=review




--- Comment #31 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 374830
  --> https://bugs.webkit.org/attachment.cgi?id=374830
Patch

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

I didn't finish yet. Will keep going tomorrow

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2385
>      const gchar* playbinName = "playbin";
>  
> -    // MSE doesn't support playbin3. Mediastream requires playbin3. Regular
> -    // playback can use playbin3 on-demand with the WEBKIT_GST_USE_PLAYBIN3
> -    // environment variable.
> -    if ((!isMediaSource() && g_getenv("WEBKIT_GST_USE_PLAYBIN3")) ||
url.protocolIs("mediastream"))
> +    // MSE and Mediastream require playbin3. Regular playback can use
playbin3 on-demand with the
> +    // WEBKIT_GST_USE_PLAYBIN3 environment variable.
> +    if ((isMediaSource() || url.protocolIs("mediastream") ||
g_getenv("WEBKIT_GST_USE_PLAYBIN3")))
>	   playbinName = "playbin3";

I think we should do a GST_INFO with the playbin name we are using just after
this. This will save questions in bugzilla about using playbin or playbin3 if
we just request the logs.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:31
5
> +    bool samplesHaveDifferentNaturalSize(GstSample* sampleA, GstSample*
sampleB) const;

doSamplesHaveDifferentNaturalSizes

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:294
> +    GRefPtr<GstCaps> caps = appendPipeline->appsinkCaps();

!

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:8
3
> +    // Only used by URI Handler API implementation

.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
05
> +    Stream(WebKitMediaSrc* source, GRefPtr<GstPad>&& pad, AtomString name,
WebCore::MediaSourceStreamTypeGStreamer type,
> +	   GRefPtr<GstCaps>&& initialCaps, GRefPtr<GstStream>&& streamInfo)

You can make this a single line.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
12
> +	       , streamingMembersDataMutex(WTFMove(initialCaps),
source->priv->startTime, source->priv->rate,
> +		  
adoptGRef(gst_event_new_stream_collection(source->priv->collection.get())))

style

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
50
> +	   bool pushedFirstBuffer { false };
> +	   bool streamStartSent { false };
> +	   bool needsSegmentEvent { true };

hasPushedFirstBuffer
wasStreamStartSent
doesNeedSegmentEvent

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
60
> +	   Condition padLinkedOrFlushed;
> +	   Condition queueChangedOrFlushed;
> +	   Deque<GRefPtr<GstMiniObject>> queue;
> +	   bool flushing { false };
> +	   bool needsToNotifyOnLowWaterLevel { false };

padLinkedOrFlushedCondition
queueChangedOrFlushedCondition
isFlushing
doesNeedToNotifyOnLowWaterLevel

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
62
> +	   guint64 durationEnqueued() const

uint64_t

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
64
> +	       // Find the first and last GstSample in the queue and subtract
their DTS

.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:1
87
> +	   bool removed { false };

wasRemoved

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
16
> +    gst_element_class_set_static_metadata(eklass, "WebKit MediaSource source
element",
> +	   "Source", "Feeds samples coming from WebKit MediaSource object",
"Igalia <aboya at igalia.com>");

Single line

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
50
> +GRefPtr<GstStreamCollection> collectionPlusStream(GstStreamCollection*
collection, GRefPtr<GstStream> stream)

addStreamToCollection and I think it would make sense to get a && for the
stream

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
55
> +    guint n = gst_stream_collection_get_size(collection);
> +    for (guint i = 0; i < n; i++) {

unsigned, unsigned

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
57
> +	   GRefPtr<GstStream> oldStream =
gst_stream_collection_get_stream(collection, i);
> +	   gst_stream_collection_add_stream(newCollection.get(),
oldStream.leakRef());

gst_stream_collection_add_stream(newCollection.get(),
gst_object_ref(gst_stream_collection_get_stream(collection, i)));

And you don't need the { }

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
64
> +GRefPtr<GstStreamCollection> collectionMinusStream(GstStreamCollection*
collection, const GstStream* stream)

removeStreamFromCollection

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
69
> +    guint n = gst_stream_collection_get_size(collection);
> +    for (guint i = 0; i < n; i++) {

unsigned, unsigned

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
94
> +void webKitMediaSrcAddStream(WebKitMediaSrc* source, AtomString name,
WebCore::MediaSourceStreamTypeGStreamer type,
> +    GRefPtr<GstCaps>&& initialCaps)

single line

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
00
> +    GRefPtr<GstStream> streamInfo =
adoptGRef(gst_stream_new(name.string().utf8().data(), initialCaps.get(),
> +	   gstStreamType(type), GST_STREAM_FLAG_SELECT));

single line

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
39
> +    // Flush the source element **and** downstream. We want to stop the
streaming thread and for that we need all
> +    // elements downstream to be idle.

single line


More information about the webkit-reviews mailing list