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.
Created attachment 330370 [details] Patch
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 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.
Committed r226357: <https://trac.webkit.org/changeset/226357>
<rdar://problem/36272737>