[webkit-reviews] review granted: [Bug 179165] [GStreamer] move MediaSample implementation out of mse/ : [Attachment 330370] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 179165: [GStreamer] move MediaSample implementation out of mse/
https://bugs.webkit.org/show_bug.cgi?id=179165

Attachment 330370: Patch

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




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 330370
  --> https://bugs.webkit.org/attachment.cgi?id=330370
Patch

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

I have a few comments, I know you are only copying the code, but it's probably
a good time to clean it up.

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:30
> +GStreamerMediaSample::GStreamerMediaSample(GstSample* sample, const
FloatSize& presentationSize, const AtomicString& trackId)

This could receive a GRefPtr<GstSample>&& instead.

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:31
> +    : MediaSample()

Is this needed?

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:36
> +    , m_size(0)

Move this to the header.

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:38
> +    , m_flags(MediaSample::IsSync)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:77
> +    GStreamerMediaSample* gstreamerMediaSample = new
GStreamerMediaSample(nullptr, presentationSize, trackId);
> +    gstreamerMediaSample->m_pts = pts;
> +    gstreamerMediaSample->m_dts = dts;
> +    gstreamerMediaSample->m_duration = duration;
> +    gstreamerMediaSample->m_flags = MediaSample::IsNonDisplaying;
> +    return adoptRef(*gstreamerMediaSample);

Since constructor is private, I would use a different constructor instead of
using the early return when sample is nullptr in the constructor.

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:95
> +    GstBuffer* buffer = gst_sample_get_buffer(m_sample.get());
> +    if (buffer) {

if (auto* buffer = gst_sample_get_buffer(m_sample.get()) {

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:104
> +    PlatformSample sample = { PlatformSample::GStreamerSampleType, {
.gstSample = m_sample.get() } };
> +    return sample;

Can't we just return { PlatformSample::GStreamerSampleType, { .gstSample =
m_sample.get() } }; ?

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.h:28
> +#include <gst/gst.h>

Would it be possible to forward declare GstSample and GstCaps? or is this
required for anything else?

> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.h:33
> +class GStreamerMediaSample : public MediaSample {

This is the GStreamer implementation of MediaSample, so I would call this
MediaSampleGStreamer instead. Also add final if no other class derives from
this one.