Summary: | [GStreamer] GStreamer Media Sample | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrique Ocaña <eocanha> | ||||||||||||||||
Component: | Platform | Assignee: | Enrique Ocaña <eocanha> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | calvaris | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 157314 | ||||||||||||||||||
Attachments: |
|
Description
Enrique Ocaña
2016-10-04 03:24:37 PDT
Created attachment 290587 [details]
Patch
Created attachment 290596 [details]
Patch
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 on attachment 290596 [details]
Patch
Wait until all the patches in 157314 are ready.
Created attachment 291759 [details]
Patch
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. Created attachment 292250 [details]
Patch
Created attachment 292405 [details]
Patch
Created attachment 292778 [details]
Patch
Created attachment 292890 [details]
Patch
Comment on attachment 292890 [details] Patch Clearing flags on attachment: 292890 Committed r207877: <http://trac.webkit.org/changeset/207877> All reviewed patches have been landed. Closing bug. |