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

[bugzilla-daemon at webkit.org]

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 374897: Patch

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




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

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

We're close to finishing, I think. Anyway, it would be interesting if Phil and
Enrique could have a look at this as well, because they know the flows better
and I do.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
772
> +	       m_videoSize = FloatSize(); // Force re-calculation in next call
to naturalSize()

.

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

doSamplesHaveDifferentNaturalSizes

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:750
> +    // As it is now, resetParserState() will cause the pads to be
disconnected, so they will later be re-added on the
> +    // next initialization segment.

Personal nit: single line

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:55
> +    GRefPtr<GstCaps>& appsinkCaps() { return m_appsinkCaps; }

Either const this of remove the &. I don't think it is a good idea to provide
just plain unprotected references.

If I can device, I'd go for the const.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h
:62
> +    void seekCompleted();

reportSeekCompleted

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h
:106
> +    bool m_pipelinePlaying = true;

m_isPipelinePlaying

>
Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp
:128
> +    bool readyForMoreSamples = m_client->isReadyForMoreSamples(trackId);

isReadyForMoreSamples

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:6
9
> +    RefPtr<Stream> streamByName(AtomString name)
> +    {
> +	   RefPtr<Stream> stream = streams.get(name);
> +	   ASSERT(stream);
> +	   return stream;

I'd take a const AtomString& and return a const RefPtr&<Stream> if possible to
avoid unnecessary refence increases.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:8
4
> +    GUniquePtr<gchar> uri;

char

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

isReadyForMoreSamples

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:2
47
> +// GstStreamCollection are immutable objects one posted. THEY MUST NOT BE
MODIFIED once they have been posted.

one -> once

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

name.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
37
> +    webKitMediaSrcStreamFlushStop(stream, FALSE);

false

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
49
> +	   GST_ERROR("Unexpected pad mode in WebKitMediaSrc");

GST_ERROR_OBJECT?

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
83
> +static void webKitMediaSrcStreamNotifyLowWaterLevel(RefPtr<Stream> stream)

I think here you can take a RefPtr<Stream>& because the way you capture it in
the lambda closure is going to copy it.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
00
> +// called with STREAM_LOCK

// Called with STREAM_LOCK.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
36
> +	   GUniquePtr<gchar> streamId(g_strdup_printf("mse/%s",
stream->name.string().utf8().data()));

char

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
37
> +	   GRefPtr<GstEvent> event =
adoptGRef(gst_event_new_stream_start(streamId.get()));

Considering that you're creating the object here ans just pushing it three
lines below, I wouln't even adopt it into a smart ptr.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
46
> +	   GRefPtr<GstEvent> event =
adoptGRef(gst_event_new_caps(streamingMembers->pendingInitialCaps.get()));

ditto

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
87
> +	       GUniquePtr<gchar> fileName {
g_strdup_printf("play-first-buffer-%s", stream->name.string().utf8().data()) };

char

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:5
05
> +	   GRefPtr<GstEvent> event =
GRefPtr<GstEvent>(GST_EVENT(object.leakRef()));
> +
> +	   streamingMembers.lockHolder().unlockEarly();
> +	   bool eventHandled = gst_pad_push_event(pad,
GRefPtr<GstEvent>(event).leakRef());

I don't think we need such convolution with the references here. I'd better do:
GstEvent* event = GST_EVENT(object.get()):
...
... gst_pad_push_event(pad, gst_event_ref(event));

I don't have a strong opinion on this though. For me it looks quite convoluted
and it makes clear that GRefPtr could have a copyRef method.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:5
12
> +static void webKitMediaSrcEnqueueObject(WebKitMediaSrc* source, AtomString
streamName, GRefPtr<GstMiniObject>&& object)

const AtomString& here and up the onion? (EnqueueSample, Event, EOS, etc.)

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:5
68
> +static void webKitMediaSrcStreamFlushStart(RefPtr<Stream>& stream)

is it possible to const RefPtr<Stream>& and the stop?

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:6
27
> +   
gst_element_query_position(findPipeline(GRefPtr<GstElement>(GST_ELEMENT(source)
)).get(), GST_FORMAT_TIME,
> +	   reinterpret_cast<gint64*>(&pipelineStreamTime));

Single line?

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:6
29
> +    // -1 is returned when the pipeline is not yet pre-rolled (e.g. just
after a seek). In this case we don't need to
> +    // adjust the segment though, as running time has not advanced.

ditto?

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:6
52
> +	   RefPtr<Stream> stream = pair.value;

Why do you need to increase the reference here? Can't you just auto& ?

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:6
81
> +    // Barring pipeline dumps someone may add during debugging, WebKit will
only read these properties
> +    // (n-video etc.) from the main thread.

single line

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.h:67
> +void webKitMediaSrcAddStream(WebKitMediaSrc*, AtomString name,
WebCore::MediaSourceStreamTypeGStreamer,
> +    GRefPtr<GstCaps>&& initialCaps);

single line?