790716 - Allocate shared YCbCr images in one Shmem instead of one per channel

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

With async-video, we are trying to reduce the number of frame copies in the video pipeline (see bug 773440). This means that we need to use more shared images than before (and non-shared images, basically the 10 images sitting in the MediaQueue), which makes it very easy to hit the maximum amount of open files on certain systems.

Right now shared YCbCr images are using a shmem per buffer. We should use only one shmem to reduce the number of open files. This could be a first step toward grouping several shared images in one shmem and do something like a circular buffer, maybe?

Comment 1

[Nicolas Silva [:nical]]

Attachment: WIP - YCbCrImage and ShmemYCbCrImage

Work in progress, I need to document the code and double check it.
I added a new ipdl type YCbCrImage that will replace YUVImage (which I intend to do in another patch), and ShmemYCbCrImage which is a wrapper around YCbCrImage.
Basically, ShmemYCbCrImage provides a convenient access to the image data without managing the memory. This will allow me to remove some awkward code in ImageBridge by avoiding some code duplication we have for allocating and copying data.

YCbCrImage is defined by a shmem and an offset (and a picture rect), the offset is always zero in this patch, but it will be used to store several images in one shmem in another patch that's to come.

I started replacing YUVImage entirely but it became a huge patch that would bitrot whatever touches the ImageLayer classes. I'd better wait for the layers refactoring to land before doing this.

Comment 2

[Nicolas Silva [:nical]]

Attachment: YCbCrImage and ShmemYCbCrImage

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 661150 [details] [diff] [review]
WIP - YCbCrImage and ShmemYCbCrImage

Review of attachment 661150 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ImageContainerChild.h
@@ +7,5 @@
>  #define MOZILLA_GFX_IMAGECONTAINERCHILD_H
>  
>  #include "mozilla/layers/PImageContainerChild.h"
>  #include "mozilla/layers/ImageBridgeChild.h"
> +#include "mozilla/layers/ShmemYCbCrImage.h"

Perhaps this only needs to be included in the .cpp

::: gfx/layers/ipc/ShmemYCbCrImage.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +// The Data is laied out as follows:

layed

@@ +24,5 @@
> +//  +-----------------+   ------+   |
> +//  |      data       |             |
> +//  +-----------------+   ----------+
> +//  |      data       |
> +//  +-----------------+

trailing whitespace

@@ +26,5 @@
> +//  +-----------------+   ----------+
> +//  |      data       |
> +//  +-----------------+
> +//
> +// There can be padding between the blocs above to keep world alignment.

blocks

@@ +30,5 @@
> +// There can be padding between the blocs above to keep world alignment.
> +
> +struct YCbCrBufferInfo
> +{
> +  size_t mYOffset;

If you can try to justify the use of offsets instead of pointers in a comment.

@@ +32,5 @@
> +struct YCbCrBufferInfo
> +{
> +  size_t mYOffset;
> +  size_t mCbOffset;
> +  size_t mCrOffset;

You can use a type that's smaller than size_t like uint32_t.

@@ +72,5 @@
> +
> +long ShmemYCbCrImage::GetCbCrStride()
> +{
> +  YCbCrBufferInfo* info = GetYCbCrBufferInfo(mShmem, mOffset);
> +  return info->mCbCrWidth;

Add a comment or fix the difference in types between Width and the return value.

::: gfx/layers/opengl/ImageLayerOGL.h
@@ +182,5 @@
>  private:
>    bool Init(const SharedImage& aFront);
>    void UploadSharedYUVToTexture(const YUVImage& yuv);
> +  void UploadSharedYCbCrToTexture(ShmemYCbCrImage& aImage,
> +                                  nsIntRect aPictureRect);

Add a comment about how YCbCrToTexture will eventually replace YUVtoTexture and why it hasn't yet.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Comment on attachment 661150 [details] [diff] [review]
> WIP - YCbCrImage and ShmemYCbCrImage
> 
> Review of attachment 661150 [details] [diff] [review]:
> -----------------------------------------------------------------


> @@ +30,5 @@
> > +// There can be padding between the blocs above to keep world alignment.
> > +
> > +struct YCbCrBufferInfo
> > +{
> > +  size_t mYOffset;
> 
> If you can try to justify the use of offsets instead of pointers in a
> comment.

I find offsets easier to reason with. Plus, as it is now the image's blob is completely position independent, meaning that it could be copied with just one memcpy if the need arises. This is actually not an unlikely scenario since we tend to share video frames to many different threads at the same time.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I haven't read the patch, but how is this supposed to work with directly-textured buffers?

Comment 6

[Nicolas Silva [:nical]]

Attachment: YCbCrImage and ShmemYCbCrImage

patch with jeff's review items fixed

Comment 7

[Nicolas Silva [:nical]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> I haven't read the patch, but how is this supposed to work with
> directly-textured buffers?

It doesn't touch the gralloc code paths at all. I don't think we should do one gralloc surface per channel unless we can bind parts of the gralloc buffer separately as gl textures at the same time (I don't know that we can do this).

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't believe we could do that, but it's also possible to write another shader that only uses one texture.  Perf impact unknown (probably bad).

Anywho, just wanted to sanity check :).

Comment 9

[Nicolas Silva [:nical]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> I don't believe we could do that, but it's also possible to write another
> shader that only uses one texture.  Perf impact unknown (probably bad).
> 
> Anywho, just wanted to sanity check :).

yeah that would do more harm than good.

Comment 10

[Nicolas Silva [:nical]]

Attachment: YCbCrImage and ShmemYCbCrImage

I had to fix an android crash on try caused by the patch, so here is the new version. It just contains some more checks to make sure that we react accordingly when the allocation of the shmem fails.

Comment 11

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9e531de8f2

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/cc9e531de8f2