767480 - Support gralloc-backed video buffers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Joe Drew (not getting mail)]

Once we have asynchronous OMTC video, we should also support using gralloc-backed buffers, at least on gonk, to make upload time significantly smaller.

Comment 1

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

Kan-Ru, can you extract the work you did to add gralloc buffer to PImageBridge and post it for review here?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Move GonkIOSurfaceImage to its own header.

This one was missed when I land..

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Support gralloc-backed video buffers in ShadowImageLayerOGL

Extracted from platform-demo-mc, not tested.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Temporary patch for testing.

Apply this patch then open the video. It would crash in either PImageContainerChild or PImageContainerParent.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Temporary patch for testing.

Reupload testing patch.

Comment 6

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

So with a --enable-debug --disable-optimize build, I see us end up at

bool
PImageContainerChild::Read(
        SurfaceDescriptorGralloc* __v,
        const Message* __msg,
        void** __iter)
{
    // skipping actor field that's meaningless on this side
    if ((!(Read((&((__v)->bufferChild())), __msg, __iter, false)))) {

but astoundingly, we're jumping to the wrong method

   0x419d12aa <+34>:	ldr	r3, [r7, #0]
   0x419d12ac <+36>:	bl	0x418df36c <mozilla::dom::PExternalHelperAppParent::Read(mozilla::dom::PExternalHelperAppParent**, IPC::Message const*, void**, bool)>
=> 0x419d12b0 <+40>:	mov	r3, r0

(gdb) f 0
#0  mozilla::dom::PExternalHelperAppParent::Read (this=0xff53e0, __v=0x47626b28, __msg=0x47626c00, __iter=0x47626b68, __nullable=false) at /home/cjones/mozilla/new-b2g/objdir-gecko/ipc/ipdl/PExternalHelperAppParent.cpp:476
(gdb) p *__v
$2 = (mozilla::layers::GrallocBufferActor *) 0x1452380


I keep segfaulting in gdb soon thereafter, not sure why.  So this is disturbing!

Comment 7

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

$ nm -g objdir-gecko/ipc/ipdl/PImageContainerChild.o | c++filt | grep External

Nothing ... that is most disturbing indeed.

Comment 8

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

I see the same thing in an --enable-debug/--enable-optimize build.  Not sure if that affects anything, or whether it's a gdb bug.

Comment 9

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

Attachment: Patch for easier debugging

Been meaning to do this forever.  Will split into separate bug.

Comment 10

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

Looks like some kind of crazy double-delete; I see this when tracing the PGrallocBuffer* dtors

 PImageBridgeParent
   OnMessageReceived
     ~PGrallocBufferParent

 PImageBridgeParent
   OnMessageReceived
     ~PGrallocBufferChild

so (unless gdb is lying to me), we're calling dtors for both sides from PImageBridgeParent.  That would be bad!

Comment 11

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

<kanru> cjones: ping
* dhylands is now known as dhylands|dinner
<kanru> cjones: I think I found the bug.. the image is reused, SetCurrentImage'ed twice

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

SetCurrentImage() on the same image twice is very reasonable behavior and should be supported.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

Comment 14

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

Note, the hard work is in bug 757341, which is finishing up the review process.  This is a small patch on top of that.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP: PlanarYCbCrImage refactoring

hide some non-public fields.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP: Gralloc backed video buffer

Add a subtype of PlanarYCbCrImage. Doesn't work yet, always get inaccessible memory from gralloc :-/

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: PlanarYCbCrImage refactoring

Hide some private variables.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Gralloc backed video buffer

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Gralloc backed video buffer. v1.1

Fill missing comment.

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=281b245981f2

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653390 [details] [diff] [review]
PlanarYCbCrImage refactoring

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

::: gfx/layers/ImageContainer.h
@@ +620,5 @@
>      gfxIntSize mCbCrSize;
> +    PRInt32 mCbOffset;
> +    PRInt32 mCbSkip;
> +    PRInt32 mCrOffset;
> +    PRInt32 mCrSkip;

You need to document what these fields are and why we need them. (They should have been documented when they were first added to CopyData.)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653392 [details] [diff] [review]
Gralloc backed video buffer. v1.1

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

::: content/media/nsBuiltinDecoderReader.cpp
@@ +68,5 @@
> +             const VideoData::YCbCrBuffer::Plane& aCrPlane)
> +{
> +  return
> +    (aYPlane.mWidth / 2) == aCbPlane.mWidth &&
> +    aCbPlane.mWidth == aCrPlane.mWidth;

Shouldn't you be checking vertical dimensions too?

::: gfx/layers/ImageContainer.cpp
@@ +53,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    img = new GrallocPlanarYCbCrImage();
> +#else
> +    if (FormatInList(aFormats, aNumFormats, ImageFormat::PLANAR_YCBCR)) {
> +      img = new PlanarYCbCrImage(aRecycleBin);

You can simplify this code so that we don't have two places that both do "new PlanarYCbCrImage".

@@ +499,5 @@
>    return imageSurface.forget().get();
>  }
>  
> +#ifdef MOZ_WIDGET_GONK
> +GrallocPlanarYCbCrImage::GrallocPlanarYCbCrImage()

Move this to its own file, say GrallocImages.cpp?

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: PlanarYCbCrImage refactoring v1.1

Add comment for new field.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Gralloc backed video buffer. v1.2

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=4e908d625e31

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653629 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.1

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

::: gfx/layers/ImageContainer.h
@@ +613,5 @@
>      gfxIntSize mYSize;
> +    // Pixels to skip between lines in the Y plane.
> +    PRInt32 mYOffset;
> +    // Pixels to skip between pixels in the Y plane.
> +    PRInt32 mYSkip;

Unfortunately these descriptions still aren't clear.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653630 [details] [diff] [review]
Gralloc backed video buffer. v1.2

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

::: gfx/layers/GrallocImages.h
@@ +24,5 @@
> + * This format assumes
> + * - an even width
> + * - an even height
> + * - a horizontal stride multiple of 16 pixels
> + * - a vertical stride equal to the height

You should add assertions to GrallocPlanarYCbCrImage::SetData to check these assumptions.

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: PlanarYCbCrImage refactoring v1.2

Move the comments of new fields to top. Hope this explains better.

Comment 29

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Gralloc backed video buffer. v1.3

Add precondition check and a memcpy fast path.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653650 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.2

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

OK, I don't want to hold this up, but I do want to have better comments for what skip and offset mean ... in a followup patch I guess. Maybe some examples?

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

Pushed with windows fixes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17ec4e01c126
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f7bace9cf9

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

And backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf66490dfad

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: PlanarYCbCrImage refactoring v1.3

patch to push

Comment 34

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Gralloc backed video buffer. v1.4

patch to push

Comment 35

[Kan-Ru Chen [:kanru] (UTC+9)]

Push again

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ec49e3076f
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f6e5a00d8b

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1ec49e3076f
https://hg.mozilla.org/mozilla-central/rev/94f6e5a00d8b

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add PlanarYCbCr buffer layout example.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654059 [details] [diff] [review]
Add PlanarYCbCr buffer layout example.

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

::: gfx/layers/ImageContainer.h
@@ +617,5 @@
> + * 0     19  22  25  28  31  34  37  40                  639       669
> + * |----------------------------------------------------------------|
> + * |%%%%%|Y___Y___Y___Y___Y___Y___Y___Y...                |%%%%%%%%%|
> + * |<--->|            |<->|
> + *    mYSkip            mYSkip

I think the first mYSkip on this line should be mYOffset.

Why is mYOffset needed? Can't the caller just add this offset to mYChannel?

Comment 39

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Comment on attachment 654059 [details] [diff] [review]
> Add PlanarYCbCr buffer layout example.
> 
> Review of attachment 654059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageContainer.h
> @@ +617,5 @@
> > + * 0     19  22  25  28  31  34  37  40                  639       669
> > + * |----------------------------------------------------------------|
> > + * |%%%%%|Y___Y___Y___Y___Y___Y___Y___Y...                |%%%%%%%%%|
> > + * |<--->|            |<->|
> > + *    mYSkip            mYSkip
> 
> I think the first mYSkip on this line should be mYOffset.
> 
> Why is mYOffset needed? Can't the caller just add this offset to mYChannel?

It is per-line offset, means each line must add that offset. I will change the example to two lines to indicate that.

Comment 40

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add PlanarYCbCr buffer layout example. v1.1

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In your example, if you were to add 19 to mYChannel and leave mYStride as 670, all lines would start 19 bytes later, so that would work.

Comment 42

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> In your example, if you were to add 19 to mYChannel and leave mYStride as
> 670, all lines would start 19 bytes later, so that would work.

Yeah, that make sense. Do you want to remove mYOffset?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I think we should remove those offsets.

Comment 44

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Remave offset field from PlanarYCbcrImage::Data.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654117 [details] [diff] [review]
Remave offset field from PlanarYCbcrImage::Data.

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

Excellent!

Comment 46

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=b7f4e114e0a0

Comment 47

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 48

[Phil Ringnalda (:philor)]

Needed a clobber on Linux64, if anyone comes by thinking that you broke mochitest-1 and reftest on their tree when it gets merged around.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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