[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