Support gralloc-backed video buffers

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: kanru)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla17
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(4 attachments, 16 obsolete attachments)

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

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → kchen
blocking-basecamp: ? → +
(Assignee)

Comment 2

5 years ago
Created attachment 643530 [details] [diff] [review]
Part 1. Move GonkIOSurfaceImage to its own header.

This one was missed when I land..
Attachment #643530 - Flags: review?(roc)
(Assignee)

Comment 3

5 years ago
Created attachment 643532 [details] [diff] [review]
Support gralloc-backed video buffers in ShadowImageLayerOGL

Extracted from platform-demo-mc, not tested.
Attachment #643532 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
Attachment #643530 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #643532 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 644010 [details] [diff] [review]
Temporary patch for testing.

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.
Created attachment 644086 [details] [diff] [review]
Patch for easier debugging

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

Comment 13

5 years ago
Created attachment 649226 [details] [diff] [review]
WIP patch
Attachment #643530 - Attachment is obsolete: true
Attachment #643532 - Attachment is obsolete: true
Attachment #644010 - Attachment is obsolete: true

Updated

5 years ago
Priority: -- → P1
(Reporter)

Updated

5 years ago
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.
(Assignee)

Comment 15

5 years ago
Created attachment 652643 [details] [diff] [review]
WIP: PlanarYCbCrImage refactoring

hide some non-public fields.
(Assignee)

Comment 16

5 years ago
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 :-/
Attachment #649226 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 653390 [details] [diff] [review]
PlanarYCbCrImage refactoring

Hide some private variables.
Attachment #652643 - Attachment is obsolete: true
Attachment #653390 - Flags: review?(roc)
(Assignee)

Comment 18

5 years ago
Created attachment 653391 [details] [diff] [review]
Gralloc backed video buffer
Attachment #652644 - Attachment is obsolete: true
Attachment #653391 - Flags: review?(roc)
(Assignee)

Comment 19

5 years ago
Created attachment 653392 [details] [diff] [review]
Gralloc backed video buffer. v1.1

Fill missing comment.
Attachment #653391 - Attachment is obsolete: true
Attachment #653391 - Flags: review?(roc)
Attachment #653392 - Flags: review?(roc)
(Assignee)

Comment 20

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=281b245981f2
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?
(Assignee)

Comment 23

5 years ago
Created attachment 653629 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.1

Add comment for new field.
Attachment #653390 - Attachment is obsolete: true
Attachment #653390 - Flags: review?(roc)
Attachment #653629 - Flags: review?(roc)
(Assignee)

Comment 24

5 years ago
Created attachment 653630 [details] [diff] [review]
Gralloc backed video buffer. v1.2
Attachment #653392 - Attachment is obsolete: true
Attachment #653392 - Flags: review?(roc)
Attachment #653630 - Flags: review?(roc)
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4e908d625e31
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.
(Assignee)

Comment 28

5 years ago
Created attachment 653650 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.2

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

Comment 29

5 years ago
Created attachment 653651 [details] [diff] [review]
Gralloc backed video buffer. v1.3

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+
Attachment #653651 - Flags: review?(roc) → review+
(Assignee)

Comment 31

5 years ago
Pushed with windows fixes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17ec4e01c126
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f7bace9cf9
Whiteboard: [LOE:S] → [LOE:S] [leave open]
(Assignee)

Comment 32

5 years ago
And backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf66490dfad
(Assignee)

Comment 33

5 years ago
Created attachment 653699 [details] [diff] [review]
PlanarYCbCrImage refactoring v1.3

patch to push
Attachment #653650 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 653700 [details] [diff] [review]
Gralloc backed video buffer. v1.4

patch to push
Attachment #653651 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
Push again

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ec49e3076f
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f6e5a00d8b
https://hg.mozilla.org/mozilla-central/rev/e1ec49e3076f
https://hg.mozilla.org/mozilla-central/rev/94f6e5a00d8b
(Assignee)

Comment 37

5 years ago
Created attachment 654059 [details] [diff] [review]
Add PlanarYCbCr buffer layout example.
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?
(Assignee)

Comment 39

5 years ago
(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.
(Assignee)

Comment 40

5 years ago
Created attachment 654066 [details] [diff] [review]
Add PlanarYCbCr buffer layout example. v1.1
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.
(Assignee)

Comment 42

5 years ago
(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.
(Assignee)

Comment 44

5 years ago
Created attachment 654117 [details] [diff] [review]
Remave offset field from PlanarYCbcrImage::Data.
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+

Updated

5 years ago
Depends on: 785001
(Assignee)

Comment 46

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

Updated

5 years ago
Depends on: 785339
Depends on: 785441
(Assignee)

Comment 47

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 786103
Depends on: 786117
(Assignee)

Updated

5 years ago
Depends on: 787048
No longer depends on: 786117
You need to log in before you can comment on or make changes to this bug.