Closed Bug 767480 Opened 8 years ago Closed 8 years ago

Support gralloc-backed video buffers

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: joe, Assigned: kanru)

References

Details

(Whiteboard: [LOE:S])

Attachments

(4 files, 16 obsolete files)

4.27 KB, patch
Details | Diff | Splinter Review
31.75 KB, patch
Details | Diff | Splinter Review
15.49 KB, patch
Details | Diff | Splinter Review
5.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
Once we have asynchronous OMTC video, we should also support using gralloc-backed buffers, at least on gonk, to make upload time significantly smaller.
Kan-Ru, can you extract the work you did to add gralloc buffer to PImageBridge and post it for review here?
Assignee: nobody → kchen
blocking-basecamp: ? → +
This one was missed when I land..
Attachment #643530 - Flags: review?(roc)
Extracted from platform-demo-mc, not tested.
Attachment #643532 - Flags: review?(jones.chris.g)
Attachment #643530 - Flags: review?(roc)
Attachment #643532 - Flags: review?(jones.chris.g)
Attached patch Temporary patch for testing. (obsolete) — Splinter Review
Apply this patch then open the video. It would crash in either PImageContainerChild or PImageContainerParent.
Attached patch Temporary patch for testing. (obsolete) — Splinter Review
Reupload testing patch.
Attachment #643997 - Attachment is obsolete: true
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!
$ nm -g objdir-gecko/ipc/ipdl/PImageContainerChild.o | c++filt | grep External

Nothing ... that is most disturbing indeed.
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.
Been meaning to do this forever.  Will split into separate bug.
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!
<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
SetCurrentImage() on the same image twice is very reasonable behavior and should be supported.
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #643530 - Attachment is obsolete: true
Attachment #643532 - Attachment is obsolete: true
Attachment #644010 - Attachment is obsolete: true
Priority: -- → P1
Whiteboard: [LOE:S]
Note, the hard work is in bug 757341, which is finishing up the review process.  This is a small patch on top of that.
hide some non-public fields.
Attached patch WIP: Gralloc backed video buffer (obsolete) — Splinter Review
Add a subtype of PlanarYCbCrImage. Doesn't work yet, always get inaccessible memory from gralloc :-/
Attachment #649226 - Attachment is obsolete: true
Attached patch PlanarYCbCrImage refactoring (obsolete) — Splinter Review
Hide some private variables.
Attachment #652643 - Attachment is obsolete: true
Attachment #653390 - Flags: review?(roc)
Attached patch Gralloc backed video buffer (obsolete) — Splinter Review
Attachment #652644 - Attachment is obsolete: true
Attachment #653391 - Flags: review?(roc)
Fill missing comment.
Attachment #653391 - Attachment is obsolete: true
Attachment #653391 - Flags: review?(roc)
Attachment #653392 - Flags: review?(roc)
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 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?
Add comment for new field.
Attachment #653390 - Attachment is obsolete: true
Attachment #653390 - Flags: review?(roc)
Attachment #653629 - Flags: review?(roc)
Attachment #653392 - Attachment is obsolete: true
Attachment #653392 - Flags: review?(roc)
Attachment #653630 - Flags: review?(roc)
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 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.
Move the comments of new fields to top. Hope this explains better.
Attachment #653629 - Attachment is obsolete: true
Attachment #653629 - Flags: review?(roc)
Attachment #653650 - Flags: review?(roc)
Add precondition check and a memcpy fast path.
Attachment #653630 - Attachment is obsolete: true
Attachment #653630 - Flags: review?(roc)
Attachment #653651 - Flags: review?(roc)
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?
Attachment #653650 - Flags: review?(roc) → review+
patch to push
Attachment #653650 - Attachment is obsolete: true
patch to push
Attachment #653651 - Attachment is obsolete: true
Attachment #654059 - Flags: review?(roc)
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?
(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.
Attachment #654059 - Attachment is obsolete: true
Attachment #654059 - Flags: review?(roc)
Attachment #654066 - Flags: review?(roc)
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.
(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?
Yes, I think we should remove those offsets.
Attachment #654066 - Attachment is obsolete: true
Attachment #654066 - Flags: review?(roc)
Attachment #654117 - Flags: review?(roc)
Comment on attachment 654117 [details] [diff] [review]
Remave offset field from PlanarYCbcrImage::Data.

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

Excellent!
Attachment #654117 - Flags: review?(roc) → review+
Depends on: 785001
Depends on: 785339
Depends on: 785441
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26d6c5a2d93
Whiteboard: [LOE:S] [leave open] → [LOE:S]
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.
Depends on: 785679
https://hg.mozilla.org/mozilla-central/rev/f26d6c5a2d93
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 786103
Depends on: 786117
Depends on: 787048
No longer depends on: 786117
You need to log in before you can comment on or make changes to this bug.