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?

This one was missed when I land..

Extracted from platform-demo-mc, not tested.

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

Reupload testing patch.

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.

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.

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

Hide some private variables.

Fill missing comment.

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.

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.

Add precondition check and a memcpy fast path.

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?

Pushed with windows fixes. https://hg.mozilla.org/integration/mozilla-inbound/rev/17ec4e01c126 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f7bace9cf9

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

patch to push

patch to push

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

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.

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.

Comment on attachment 654117 [details] [diff] [review] Remave offset field from PlanarYCbcrImage::Data. Review of attachment 654117 [details] [diff] [review]: ----------------------------------------------------------------- Excellent!

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.