Bug 162894 - [GStreamer] GStreamer Media Sample
Summary: [GStreamer] GStreamer Media Sample
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords:
Depends on:
Blocks: 157314
  Show dependency treegraph
 
Reported: 2016-10-04 03:24 PDT by Enrique Ocaña
Modified: 2016-10-26 01:42 PDT (History)
1 user (show)

See Also:


Attachments
Patch (9.46 KB, patch)
2016-10-04 03:27 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2016-10-04 05:45 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2016-10-16 12:04 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (9.32 KB, patch)
2016-10-20 13:55 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (9.28 KB, patch)
2016-10-21 14:23 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (10.13 KB, patch)
2016-10-25 11:02 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (10.13 KB, patch)
2016-10-26 01:19 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2016-10-04 03:24:37 PDT
Implement the MediaSample interface for the GStreamer platform.
Comment 1 Enrique Ocaña 2016-10-04 03:27:51 PDT
Created attachment 290587 [details]
Patch
Comment 2 Enrique Ocaña 2016-10-04 05:45:14 PDT
Created attachment 290596 [details]
Patch
Comment 3 Xabier Rodríguez Calvar 2016-10-04 07:02:04 PDT
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.
Comment 4 Enrique Ocaña 2016-10-04 07:06:43 PDT
Comment on attachment 290596 [details]
Patch

Wait until all the patches in 157314 are ready.
Comment 5 Enrique Ocaña 2016-10-16 12:04:11 PDT
Created attachment 291759 [details]
Patch
Comment 6 Zan Dobersek 2016-10-18 01:18:53 PDT
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.
Comment 7 Enrique Ocaña 2016-10-20 13:55:03 PDT
Created attachment 292250 [details]
Patch
Comment 8 Enrique Ocaña 2016-10-21 14:23:46 PDT
Created attachment 292405 [details]
Patch
Comment 9 Enrique Ocaña 2016-10-25 11:02:39 PDT
Created attachment 292778 [details]
Patch
Comment 10 Enrique Ocaña 2016-10-26 01:19:18 PDT
Created attachment 292890 [details]
Patch
Comment 11 Enrique Ocaña 2016-10-26 01:42:02 PDT
Comment on attachment 292890 [details]
Patch

Clearing flags on attachment: 292890

Committed r207877: <http://trac.webkit.org/changeset/207877>
Comment 12 Enrique Ocaña 2016-10-26 01:42:10 PDT
All reviewed patches have been landed.  Closing bug.