Closed Bug 761018 Opened 8 years ago Closed 7 years ago

GStreamer video buffer handling optimization

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: alessandro.d, Assigned: alessandro.d)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #629688 - Flags: review?(chris.double)
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?
(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.
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)
Attachment #632603 - Flags: review?(chris.double)
Attachment #632603 - Flags: review?(chris.double) → review+
Attached patch update (obsolete) — Splinter Review
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 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-
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)
Attached patch update to Aurora 20 (obsolete) — Splinter Review
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)
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 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+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch review fixesSplinter Review
Attachment #723103 - Attachment is obsolete: true
I changed the things you pointed out. Thanks a lot for this and all the other reviews Chris.
https://hg.mozilla.org/mozilla-central/rev/7c3e470dc0eb
Assignee: nobody → alessandro.d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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?
Not really. I must have accidentally done it resolving conflicts, I'll fix this.
I guess we need new bug and dependency in order to fix it properly
Oh, it was already fixed here https://bugzilla.mozilla.org/show_bug.cgi?id=851906
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')
ah, ok can be solved with simple headers update for gtype.h and gstbuffer.h it works
Attached patch Fix broken compilation (obsolete) — Splinter Review
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.
Depends on: 854539
Attachment #728141 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.