Bug 179165 – [GStreamer] move MediaSample implementation out of mse/

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

This isn't specific to MSE and can be reused elsewhere (for WebRTC). So it should be moved one directory down. Also the ::platformMediaSample() method should be easy to implement.

[Philippe Normand]

Created attachment 330370 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 330370 [details]
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.

[Philippe Normand]

Comment on attachment 330370 [details]
Patch

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

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

Likely not!

>> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:36
>> +    , m_size(0)
> 
> Move this to the header.

Sure!

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

Ok!

>> Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:77
>> +    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.

Ok!

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

Ok!

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

../../Source/WebCore/platform/graphics/gstreamer/GStreamerMediaSample.cpp:98:83: internal compiler error: in reshape_init_class, at cp/decl.c:5802
     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?

Forward declaration is already done in GRefPtrGStreamer.h. But yeah you are right.

>> 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.

Ok, I'll also rename the files then.

[Philippe Normand]

Committed r226357: <https://trac.webkit.org/changeset/226357>

[Radar WebKit Bug Importer]

<rdar://problem/36272737>