Last Comment Bug 767480 - Support gralloc-backed video buffers
: Support gralloc-backed video buffers
Status: RESOLVED FIXED
[LOE:S]
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: P1 normal (vote)
: mozilla17
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on: 785441 787048 598868 765734 785001 785339 785679 786103
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-22 11:24 PDT by Joe Drew (not getting mail)
Modified: 2012-09-06 18:44 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Part 1. Move GonkIOSurfaceImage to its own header. (6.01 KB, patch)
2012-07-18 12:51 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Support gralloc-backed video buffers in ShadowImageLayerOGL (22.78 KB, patch)
2012-07-18 12:53 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Temporary patch for testing. (32.67 KB, patch)
2012-07-19 13:39 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Temporary patch for testing. (36.09 KB, patch)
2012-07-19 14:14 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Patch for easier debugging (4.27 KB, patch)
2012-07-19 17:35 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
WIP patch (102.46 KB, patch)
2012-08-06 03:06 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
WIP: PlanarYCbCrImage refactoring (10.73 KB, patch)
2012-08-16 18:45 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
WIP: Gralloc backed video buffer (13.26 KB, patch)
2012-08-16 18:46 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
PlanarYCbCrImage refactoring (11.52 KB, patch)
2012-08-20 08:16 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Gralloc backed video buffer (12.96 KB, patch)
2012-08-20 08:17 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Gralloc backed video buffer. v1.1 (13.16 KB, patch)
2012-08-20 08:24 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
PlanarYCbCrImage refactoring v1.1 (21.82 KB, patch)
2012-08-20 19:34 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Gralloc backed video buffer. v1.2 (14.72 KB, patch)
2012-08-20 19:34 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
PlanarYCbCrImage refactoring v1.2 (26.33 KB, patch)
2012-08-20 21:58 PDT, Kan-Ru Chen [:kanru] (UTC+8)
roc: review+
Details | Diff | Review
Gralloc backed video buffer. v1.3 (15.48 KB, patch)
2012-08-20 21:59 PDT, Kan-Ru Chen [:kanru] (UTC+8)
roc: review+
Details | Diff | Review
PlanarYCbCrImage refactoring v1.3 (31.75 KB, patch)
2012-08-21 03:25 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Gralloc backed video buffer. v1.4 (15.49 KB, patch)
2012-08-21 03:25 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Add PlanarYCbCr buffer layout example. (1.43 KB, patch)
2012-08-21 20:10 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Add PlanarYCbCr buffer layout example. v1.1 (1.57 KB, patch)
2012-08-21 20:50 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Remave offset field from PlanarYCbcrImage::Data. (5.88 KB, patch)
2012-08-22 00:07 PDT, Kan-Ru Chen [:kanru] (UTC+8)
roc: review+
Details | Diff | Review

Description Joe Drew (not getting mail) 2012-06-22 11:24:59 PDT
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 2012-07-14 19:38:09 PDT
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+8) 2012-07-18 12:51:45 PDT
Created attachment 643530 [details] [diff] [review]
Part 1. Move GonkIOSurfaceImage to its own header.

This one was missed when I land..
Comment 3 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-18 12:53:26 PDT
Created attachment 643532 [details] [diff] [review]
Support gralloc-backed video buffers in ShadowImageLayerOGL

Extracted from platform-demo-mc, not tested.
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-19 13:39:32 PDT
Created attachment 643997 [details] [diff] [review]
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+8) 2012-07-19 14:14:12 PDT
Created attachment 644010 [details] [diff] [review]
Temporary patch for testing.

Reupload testing patch.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 15:59:03 PDT
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 2012-07-19 16:01:33 PDT
$ 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 2012-07-19 16:33:46 PDT
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 2012-07-19 17:35:00 PDT
Created attachment 644086 [details] [diff] [review]
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 2012-07-19 18:39:27 PDT
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 2012-07-21 00:20:26 PDT
<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) (Exited; email my personal email if necessary) 2012-07-21 07:25:59 PDT
SetCurrentImage() on the same image twice is very reasonable behavior and should be supported.
Comment 13 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-06 03:06:11 PDT
Created attachment 649226 [details] [diff] [review]
WIP patch
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-16 13:49:12 PDT
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+8) 2012-08-16 18:45:01 PDT
Created attachment 652643 [details] [diff] [review]
WIP: PlanarYCbCrImage refactoring

hide some non-public fields.
Comment 16 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-16 18:46:11 PDT
Created attachment 652644 [details] [diff] [review]
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+8) 2012-08-20 08:16:41 PDT
Created attachment 653390 [details] [diff] [review]
PlanarYCbCrImage refactoring

Hide some private variables.
Comment 18 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 08:17:24 PDT
Created attachment 653391 [details] [diff] [review]
Gralloc backed video buffer
Comment 19 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 08:24:17 PDT
Created attachment 653392 [details] [diff] [review]
Gralloc backed video buffer. v1.1

Fill missing comment.
Comment 20 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 09:04:48 PDT
https://tbpl.mozilla.org/?tree=Try&rev=281b245981f2
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 14:51:32 PDT
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) (Exited; email my personal email if necessary) 2012-08-20 16:30:18 PDT
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+8) 2012-08-20 19:34:00 PDT
Created attachment 653629 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.1

Add comment for new field.
Comment 24 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 19:34:40 PDT
Created attachment 653630 [details] [diff] [review]
Gralloc backed video buffer. v1.2
Comment 25 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 19:34:53 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4e908d625e31
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 20:42:50 PDT
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) (Exited; email my personal email if necessary) 2012-08-20 20:45:29 PDT
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+8) 2012-08-20 21:58:26 PDT
Created attachment 653650 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.2

Move the comments of new fields to top. Hope this explains better.
Comment 29 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-20 21:59:25 PDT
Created attachment 653651 [details] [diff] [review]
Gralloc backed video buffer. v1.3

Add precondition check and a memcpy fast path.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 22:49:04 PDT
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 32 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-21 02:59:10 PDT
And backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf66490dfad
Comment 33 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-21 03:25:21 PDT
Created attachment 653699 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.3

patch to push
Comment 34 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-21 03:25:47 PDT
Created attachment 653700 [details] [diff] [review]
Gralloc backed video buffer. v1.4

patch to push
Comment 37 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-21 20:10:14 PDT
Created attachment 654059 [details] [diff] [review]
Add PlanarYCbCr buffer layout example.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 20:37:20 PDT
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+8) 2012-08-21 20:42:04 PDT
(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+8) 2012-08-21 20:50:49 PDT
Created attachment 654066 [details] [diff] [review]
Add PlanarYCbCr buffer layout example. v1.1
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 21:07:51 PDT
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+8) 2012-08-21 21:14:51 PDT
(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) (Exited; email my personal email if necessary) 2012-08-21 21:23:17 PDT
Yes, I think we should remove those offsets.
Comment 44 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-22 00:07:56 PDT
Created attachment 654117 [details] [diff] [review]
Remave offset field from PlanarYCbcrImage::Data.
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-22 16:37:43 PDT
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+8) 2012-08-23 07:47:57 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b7f4e114e0a0
Comment 47 Kan-Ru Chen [:kanru] (UTC+8) 2012-08-25 21:06:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26d6c5a2d93
Comment 48 Phil Ringnalda (:philor) 2012-08-25 23:47:47 PDT
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] 2012-08-26 13:06:01 PDT
https://hg.mozilla.org/mozilla-central/rev/f26d6c5a2d93

Note You need to log in before you can comment on or make changes to this bug.