Bug 179165 - [GStreamer] move MediaSample implementation out of mse/
Summary: [GStreamer] move MediaSample implementation out of mse/
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-02 03:55 PDT by Philippe Normand
Modified: 2018-01-03 02:59 PST (History)
3 users (show)

See Also:


Attachments
Patch (21.06 KB, patch)
2018-01-03 01:01 PST, Philippe Normand
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2017-11-02 03:55:55 PDT
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.
Comment 1 Philippe Normand 2018-01-03 01:01:10 PST
Created attachment 330370 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-01-03 01:30:47 PST
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.
Comment 3 Philippe Normand 2018-01-03 02:18:27 PST
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.
Comment 4 Philippe Normand 2018-01-03 02:58:48 PST
Committed r226357: <https://trac.webkit.org/changeset/226357>
Comment 5 Radar WebKit Bug Importer 2018-01-03 02:59:33 PST
<rdar://problem/36272737>