Bug 162894 – [GStreamer] GStreamer Media Sample

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

Implement the MediaSample interface for the GStreamer platform.

[Enrique Ocaña]

Created attachment 290587 [details]
Patch

[Enrique Ocaña]

Created attachment 290596 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 290596 [details]
Patch

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

This can be fixed at landing time.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:78
> +    GstSample* gstSample = nullptr;
> +    GStreamerMediaSample* gstreamerMediaSample = new GStreamerMediaSample(gstSample, presentationSize, trackId);

Can't you just GStreamerMediaSample* gstreamerMediaSample = new GStreamerMediaSample(nullptr, presentationSize, trackId); ?

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.h:36
> +    MediaTime m_pts, m_dts, m_duration;

Three different lines, please.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.h:65
> +    MediaTime presentationTime() const { return m_pts; }
> +    MediaTime decodeTime() const { return m_dts; }
> +    MediaTime duration() const { return m_duration; }
> +    AtomicString trackID() const { return m_trackId; }
> +    void setTrackID(const String& trackId) { m_trackId = trackId; }
> +    size_t sizeInBytes() const { return m_size; }
> +    GstSample* sample() const { return m_sample.get(); }
> +    FloatSize presentationSize() const { return m_presentationSize; }
> +    void offsetTimestampsBy(const MediaTime&);
> +    void setTimestamps(const MediaTime&, const MediaTime&) { }
> +    bool isDivisable() const { return false; }
> +    std::pair<RefPtr<MediaSample>, RefPtr<MediaSample>> divide(const MediaTime&) { return { nullptr, nullptr }; }
> +    SampleFlags flags() const { return m_flags; }
> +    PlatformSample platformSample() { return PlatformSample(); }

You need to add the override around here.

[Enrique Ocaña]

Comment on attachment 290596 [details]
Patch

Wait until all the patches in 157314 are ready.

[Enrique Ocaña]

Created attachment 291759 [details]
Patch

[Zan Dobersek]

Comment on attachment 291759 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:67
> +        m_flags = (MediaSample::SampleFlags) (m_flags | MediaSample::NonDisplaying);

I think a static_cast<MediaSample::SampleFlags>() should be preferred here.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:73
> +PassRefPtr<GStreamerMediaSample> GStreamerMediaSample::create(GstSample* sample, const FloatSize& presentationSize, const AtomicString& trackId)
> +{
> +    return adoptRef(new GStreamerMediaSample(sample, presentationSize, trackId));
> +}

This should return Ref<GStreamerMediaSample>. The whole function should be put into the header.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:75
> +PassRefPtr<GStreamerMediaSample> GStreamerMediaSample::createFakeSample(GstCaps*, MediaTime pts, MediaTime dts, MediaTime duration, const FloatSize& presentationSize, const AtomicString& trackId)

This should return Ref<GStreamerMediaSample>. This fits more into the implementation file though, so don't move it into a header.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:108
> +GStreamerMediaSample::~GStreamerMediaSample()
> +{
> +}

This should be placed directly after the constructor, and should be defaulted.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaSample.h:44
> +private:
> +    MediaTime m_pts;
> +    MediaTime m_dts;
> +    MediaTime m_duration;
> +    AtomicString m_trackId;
> +    size_t m_size;
> +    GRefPtr<GstSample> m_sample;
> +    FloatSize m_presentationSize;
> +    MediaSample::SampleFlags m_flags;
> +
> +    GStreamerMediaSample(GstSample*, const FloatSize& presentationSize, const AtomicString& trackId);

The private section always follows the public one. And the constructor is normally the first thing declared under it.

[Enrique Ocaña]

Created attachment 292250 [details]
Patch

[Enrique Ocaña]

Created attachment 292405 [details]
Patch

[Enrique Ocaña]

Created attachment 292778 [details]
Patch

[Enrique Ocaña]

Created attachment 292890 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292890 [details]
Patch

Clearing flags on attachment: 292890

Committed r207877: <http://trac.webkit.org/changeset/207877>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.