761018 - GStreamer video buffer handling optimization

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alessandro Decina]

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.

Comment 1

[Alessandro Decina]

Attachment: patch

Comment 2

[cajbir (:cajbir)]

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?

Comment 3

[Alessandro Decina]

(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.

Comment 4

[Alessandro Decina]

Attachment: new approach only for gst >= 0.10.36

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.

Comment 5

[Tobias Netzel]

Attachment: update

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.

Comment 6

[cajbir (:cajbir)]

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.

Comment 7

[Tobias Netzel]

Attachment: fixed up the mentioned points and added the missing calls to avoid the hacks

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();

Comment 8

[Tobias Netzel]

Attachment: update to Aurora 20

Comment 9

[Alessandro Decina]

Attachment: GStreamer video buffer handling optimization

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!

Comment 10

[cajbir (:cajbir)]

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.

Comment 11

[Alessandro Decina]

Attachment: review fixes

Comment 12

[Alessandro Decina]

I changed the things you pointed out. Thanks a lot for this and all the other reviews Chris.

Comment 13

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3e470dc0eb

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7c3e470dc0eb

Comment 15

[Ivaylo Dimitrov]

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?

Comment 16

[Alessandro Decina]

Not really. I must have accidentally done it resolving conflicts, I'll fix this.

Comment 17

[Oleg Romashin (:romaxa)]

I guess we need new bug and dependency in order to fix it properly

Comment 18

[Alessandro Decina]

Oh, it was already fixed here https://bugzilla.mozilla.org/show_bug.cgi?id=851906

Comment 19

[Oleg Romashin (:romaxa)]

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

[Oleg Romashin (:romaxa)]

ah, ok can be solved with simple headers update for gtype.h and gstbuffer.h it works

Comment 21

[Ivaylo Dimitrov]

Attachment: Fix broken compilation

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.