Closed
Bug 761018
Opened 12 years ago
Closed 11 years ago
GStreamer video buffer handling optimization
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: alessandro.d, Assigned: alessandro.d)
References
Details
Attachments
(1 file, 7 obsolete files)
51.00 KB,
patch
|
Details | Diff | Splinter Review |
Today in nsGStreamerReader, each YUV video frame is allocated internally by the gst pipeline and then copied in a PlanarYCbCrImage before being passed to the gfx stack. The attached patch makes the pipeline allocate PlanarYCbCrImage-based buffers directly, so that one allocation + one memcpy can be avoided for each frame.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #629688 -
Flags: review?(chris.double)
Comment 2•12 years ago
|
||
Comment on attachment 629688 [details] [diff] [review] patch Review of attachment 629688 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gstreamer/nsGStreamerReader.h @@ +154,5 @@ > + /* gBuffers is an image->mBuffer => image map that we use to lookup the > + * underlying PlanarYCbCrImage instance from a GST_BUFFER_DATA(buf) pointer > + */ > + static ReentrantMonitor gBuffersMonitor; > + typedef std::map<guint8*,nsRefPtr<PlanarYCbCrImage> > BuffersMap; How is data removed from the map at shutdown? Will this cause leaks in the leak reporting tools? I can't test since my gstreamer is too old unfortunately. Does it need to be static? The image buffers don't need to be shared across video elements do they? ::: content/media/nsBuiltinDecoderReader.h @@ +152,5 @@ > + // data in aBuffer. aTimecode is a codec specific number representing the > + // timestamp of the frame of video data. Returns nsnull if an error occurs. > + // This may indicate that memory couldn't be allocated to create the VideoData > + // object, or it may indicate some problem with the input data (e.g. negative > + // stride). Comment explains what a NULL aImage means, but not what a non-NULL aImage means. Can you add this case?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Chris Double (:doublec) from comment #2) > Comment on attachment 629688 [details] [diff] [review] > patch > > Review of attachment 629688 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/gstreamer/nsGStreamerReader.h > @@ +154,5 @@ > > + /* gBuffers is an image->mBuffer => image map that we use to lookup the > > + * underlying PlanarYCbCrImage instance from a GST_BUFFER_DATA(buf) pointer > > + */ > > + static ReentrantMonitor gBuffersMonitor; > > + typedef std::map<guint8*,nsRefPtr<PlanarYCbCrImage> > BuffersMap; > > How is data removed from the map at shutdown? Will this cause leaks in the > leak reporting tools? Items are popped from gBuffers as GstBuffers get free'd by the pipeline. So, barring GstBuffer leaks, the gBuffers map will be empty once the last pipeline is set to GST_STATE_NULL. The gBuffers std::map itself is never free'd explicitly, so I guess that will get reported as a (reachable) leak by valgrind and friends. I can't test since my gstreamer is too old > unfortunately. Does it need to be static? The image buffers don't need to be > shared across video elements do they? Correct, buffers don't need to be shared. The map currently is static as the callback that gets called from gst to free buffers has the following signature: void nsGStreamerReader::FreeVideoBufferCb(gpointer aData); So we don't have a way to pass around nsGStreamerReader instance pointers. Although, now that I think of it, I could use a function object for this. I'll try that. > ::: content/media/nsBuiltinDecoderReader.h > @@ +152,5 @@ > > + // data in aBuffer. aTimecode is a codec specific number representing the > > + // timestamp of the frame of video data. Returns nsnull if an error occurs. > > + // This may indicate that memory couldn't be allocated to create the VideoData > > + // object, or it may indicate some problem with the input data (e.g. negative > > + // stride). > > Comment explains what a NULL aImage means, but not what a non-NULL aImage > means. Can you add this case? Ok.
Assignee | ||
Comment 4•11 years ago
|
||
Got rid of the global std::map and I'm now using the new gst_buffer_set_qdata API to store the PlanarYCbCrImage pointer along with its corresponding GstBuffer. gst_buffer_set_qdata was added in gstreamer core 0.10.36, so for < 0.36, the optimization is now disabled.
Attachment #629688 -
Attachment is obsolete: true
Attachment #629688 -
Flags: review?(chris.double)
Assignee | ||
Updated•11 years ago
|
Attachment #632603 -
Flags: review?(chris.double)
Updated•11 years ago
|
Attachment #632603 -
Flags: review?(chris.double) → review+
Comment 5•11 years ago
|
||
I updated this but had to introduce some hacks (temporarily redefining protected to public). I think there must be a better and cleaner way to do this in the current code base.
Attachment #683940 -
Flags: feedback?(chris.double)
Attachment #683940 -
Flags: feedback?(alessandro.d)
Comment 6•11 years ago
|
||
Comment on attachment 683940 [details] [diff] [review] update Review of attachment 683940 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderReader.cpp @@ +5,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "GonkIOSurfaceImage.h" > +#define protected public What protected members are you needing to access? You should create a public API on MediaDecoderReader to do what you want rather than access them directly. Can you explain more what it is you need to access and why? @@ +206,5 @@ > const YCbCrBuffer::Plane &Cb = aBuffer.mPlanes[1]; > const YCbCrBuffer::Plane &Cr = aBuffer.mPlanes[2]; > > + if (aImage == NULL) { > + // Currently our decoder only knows how to output to PLANAR_YCBCR Use !aImage instead of == NULL. @@ +244,4 @@ > data.mStereoMode = aInfo.mStereoMode; > > videoImage->SetDelayedConversion(true); > + if (aImage == NULL) { Use !aImage. @@ +256,5 @@ > > +VideoData* VideoData::Create(nsVideoInfo& aInfo, > + ImageContainer* aContainer, > + int64_t aOffset, > + int64_t aTime, Remove trailing white space. @@ +263,5 @@ > + bool aKeyframe, > + int64_t aTimecode, > + nsIntRect aPicture) > +{ > + return Create(aInfo, aContainer, NULL, aOffset, aTime, aEndTime, aBuffer, Replace NULL with nullptr. @@ +264,5 @@ > + int64_t aTimecode, > + nsIntRect aPicture) > +{ > + return Create(aInfo, aContainer, NULL, aOffset, aTime, aEndTime, aBuffer, > + aKeyframe, aTimecode, aPicture); Align aKeyframe with aInfo. @@ +266,5 @@ > +{ > + return Create(aInfo, aContainer, NULL, aOffset, aTime, aEndTime, aBuffer, > + aKeyframe, aTimecode, aPicture); > +} > + Trailing whitespace. @@ +277,5 @@ > + bool aKeyframe, > + int64_t aTimecode, > + nsIntRect aPicture) > +{ > + return Create(aInfo, NULL, aImage, aOffset, aTime, aEndTime, aBuffer, Replace NULL with nullptr. @@ +278,5 @@ > + int64_t aTimecode, > + nsIntRect aPicture) > +{ > + return Create(aInfo, NULL, aImage, aOffset, aTime, aEndTime, aBuffer, > + aKeyframe, aTimecode, aPicture); Align aKeyframe with aInfo. ::: content/media/MediaDecoderReader.h @@ +132,5 @@ > Plane mPlanes[3]; > }; > > + // Constructs a VideoData object. If aImage is NULL, makes a copy of YCbCr > + // data in aBuffer, else uses aImage as the pixel a aTimecode is a codec specific number representing the This comment line seems wrong. The "as the pixel a aTimecode" seems to show something got cut off somewhere. ::: content/media/gstreamer/GStreamerReader.cpp @@ +9,2 @@ > #include "MediaDecoderStateMachine.h" > +#undef protected What members do you need access to? If you explain what and why you need it we can add an API for it. @@ +465,5 @@ > /* no more frames */ > return false; > > + nsRefPtr<PlanarYCbCrImage> image; > + while (image == NULL) { Use !image @@ +768,5 @@ > > +GstFlowReturn GStreamerReader::AllocateVideoBufferCb(GstPad *aPad, > + guint64 aOffset, guint aSize, GstCaps *aCaps, GstBuffer **aBuf) > +{ > + GStreamerReader *reader = (GStreamerReader *) gst_pad_get_element_private(aPad); Use C++ casts. In this case reinterpret_cast<> @@ +779,5 @@ > +#if GST_VERSION_MICRO >= 36 > + /* allocate an image using the container */ > + ImageContainer *container = mDecoder->GetImageContainer(); > + ImageFormat format = PLANAR_YCBCR; > + PlanarYCbCrImage *img = (PlanarYCbCrImage *) container->CreateImage(&format, 1).get(); Use C++ casts. ::: content/media/gstreamer/GStreamerReader.h @@ +15,5 @@ > class nsTimeRanges; > > namespace mozilla { > > +using namespace layers; Don't use "using namespace" in headers. Prefix layers:: to the types that need it.
Attachment #683940 -
Flags: feedback?(chris.double) → feedback-
Comment 7•11 years ago
|
||
Thanks for reviewing! By requesting just feedback I didn't expect that. In this reworked version I added the calls that are needed to avoid the hacks. I wonder whether this is actually safe to do, looking at the comment to setData();
Attachment #683940 -
Attachment is obsolete: true
Attachment #683940 -
Flags: feedback?(alessandro.d)
Attachment #686851 -
Flags: feedback?(chris.double)
Attachment #686851 -
Flags: feedback?(alessandro.d)
Comment 8•11 years ago
|
||
Attachment #686851 -
Attachment is obsolete: true
Attachment #686851 -
Flags: feedback?(chris.double)
Attachment #686851 -
Flags: feedback?(alessandro.d)
Attachment #703841 -
Flags: feedback?(chris.double)
Attachment #703841 -
Flags: feedback?(alessandro.d)
Assignee | ||
Comment 9•11 years ago
|
||
Rebased against today's m-c. I haven't tested that it compiles with <= 0.10.35 but it should. I did test that everything still works without the optimization. No new tests fail with the patch applied. Thank you again Tobias for helping move this forward!
Attachment #632603 -
Attachment is obsolete: true
Attachment #703841 -
Attachment is obsolete: true
Attachment #703841 -
Flags: feedback?(chris.double)
Attachment #703841 -
Flags: feedback?(alessandro.d)
Attachment #723103 -
Flags: review?(chris.double)
Comment 10•11 years ago
|
||
Comment on attachment 723103 [details] [diff] [review] GStreamer video buffer handling optimization Review of attachment 723103 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderReader.h @@ +132,5 @@ > Plane mPlanes[3]; > }; > > + // Constructs a VideoData object. If aImage is NULL, makes a copy of YCbCr > + // data in aBuffer, else uses aImage as the pixel a aTimecode is a codec This comment seems to be a bit mangled. ::: content/media/gstreamer/GStreamerReader.cpp @@ +771,5 @@ > return TRUE; > } > > +GstFlowReturn GStreamerReader::AllocateVideoBufferFull(GstPad* aPad, > + guint64 aOffset, Align indentation with the "GstPad" above, same with arguments below. ::: gfx/layers/ImageContainer.cpp @@ +499,5 @@ > + // update buffer size > + mBufferSize = aSize; > + > + // get new buffer > + mBuffer = AllocateBuffer(mBufferSize); Remove trailing white space. ::: gfx/layers/ImageContainer.h @@ +688,5 @@ > */ > virtual void SetData(const Data& aData); > > /** > + * This doesn't make a copy of the data buffers Can you add to this comment to make mention of why one would use this instead of SetData. Note the GStreamer backend's use of it basically.
Attachment #723103 -
Flags: review?(chris.double) → review+
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #723103 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
I changed the things you pointed out. Thanks a lot for this and all the other reviews Chris.
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7c3e470dc0eb
Assignee: nobody → alessandro.d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 15•11 years ago
|
||
As a result https://hg.mozilla.org/mozilla-central/rev/041328ec9651 is effectively backed out in it's code part - https://hg.mozilla.org/mozilla-central/rev/7c3e470dc0eb#l3.187 Any particular reason for that?
Assignee | ||
Comment 16•11 years ago
|
||
Not really. I must have accidentally done it resolving conflicts, I'll fix this.
Comment 17•11 years ago
|
||
I guess we need new bug and dependency in order to fix it properly
Assignee | ||
Comment 18•11 years ago
|
||
Oh, it was already fixed here https://bugzilla.mozilla.org/show_bug.cgi?id=851906
Comment 19•11 years ago
|
||
content/media/gstreamer/GStreamerReader.cpp: In function 'GType mozilla::buffer_data_get_type()': /content/media/gstreamer/GStreamerReader.cpp:41: error: types may not be defined in parameter types /content/media/gstreamer/GStreamerReader.cpp:41: error: types may not be defined in parameter types /content/media/gstreamer/GStreamerReader.cpp:41: error: invalid conversion from 'GType (*)(const gchar*, void* (*)(void*), void (*)(void*))' to 'GType (*)(const gchar*, mozilla::buffer_data_get_type()::<anonymous union>, mozilla::buffer_data_get_type()::<anonymous union>)' /content/media/gstreamer/GStreamerReader.cpp:41: error: conversion from 'void* (*)(void*)' to non-scalar type 'mozilla::buffer_data_get_type()::<anonymous union>' requested /content/media/gstreamer/GStreamerReader.cpp: In member function 'virtual bool mozilla::GStreamerReader::DecodeVideoFrame(bool&, int64_t)': /content/media/gstreamer/GStreamerReader.cpp:498: error: invalid conversion from 'int' to 'GstBufferCopyFlags' /content/media/gstreamer/GStreamerReader.cpp:498: error: initializing argument 3 of 'void gst_buffer_copy_metadata(GstBuffer*, const GstBuffer*, GstBufferCopyFlags)' gcc version 4.4.1 ('cs2009q3-hard-67-sb16')
Comment 20•11 years ago
|
||
ah, ok can be solved with simple headers update for gtype.h and gstbuffer.h it works
Comment 21•11 years ago
|
||
I'd propose that patch, instead of tweaking /usr/include/* :). Also, keep in mind that G_DEFINE_BOXED_TYPE appears in glib 2.26, but the latest requirements I was able to find here http://www.mozilla.org/en-US/firefox/system-requirements.html state 2.14 as minimum glib version.
Updated•11 years ago
|
Attachment #728141 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•