1113769 - Handle out of gralloc in MediaEngineGonkVideoSource::RotateImage()

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1043558 +++

Bug 1043558 enabled gralloc buffer usage for gonk camera preview. But it does not handle out of gralloc buffer case. It seems better to add its handling.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle out of gralloc

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle out of gralloc

Comment 3

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19db0349dfe4
https://tbpl.mozilla.org/?tree=Try&rev=19db0349dfe4

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8539531 [details] [diff] [review]
patch - Handle out of gralloc

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

::: dom/media/webrtc/MediaEngineGonkVideoSource.cpp
@@ +702,5 @@
> +    uint8_t* dstPtr = static_cast<uint8_t*>(destMem);
> +
> +    int32_t yStride = destBuffer->getStride();
> +    // Align to 16 bytes boundary
> +    int32_t uvStride = ((yStride / 2) + 15) & ~0x0F;

Just for paranoia, please add asserts validating widths/heights are even.

@@ +736,5 @@
> +                          aWidth, aHeight,
> +                          static_cast<libyuv::RotationMode>(mRotation),
> +                          ConvertPixelFormatToFOURCC(graphicBuffer->getPixelFormat()));
> +
> + 

space

@@ +746,5 @@
> +    data.mYSize = IntSize(dstWidth, dstHeight);
> +    data.mYStride = dstWidth * lumaBpp / 8;
> +    data.mCbCrStride = dstWidth * chromaBpp / 8;
> +    data.mCbChannel = dstPtr + dstHeight * data.mYStride;
> +    data.mCrChannel = data.mCbChannel +( dstHeight * data.mCbCrStride / 2);

spaces: " + (dstHeight"
Also, it's misleading.  Should be (assuming asserts are used):
data.mCbChannel + data.mCbCrStride * (dstHeight/2);

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Randell Jesup [:jesup] from comment #4)
> Comment on attachment 8539531 [details] [diff] [review]
> patch - Handle out of gralloc
> 
> Review of attachment 8539531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webrtc/MediaEngineGonkVideoSource.cpp
> @@ +702,5 @@
> > +    uint8_t* dstPtr = static_cast<uint8_t*>(destMem);
> > +
> > +    int32_t yStride = destBuffer->getStride();
> > +    // Align to 16 bytes boundary
> > +    int32_t uvStride = ((yStride / 2) + 15) & ~0x0F;
> 
> Just for paranoia, please add asserts validating widths/heights are even.

It is already exit in the function.

> 
> @@ +736,5 @@
> > +                          aWidth, aHeight,
> > +                          static_cast<libyuv::RotationMode>(mRotation),
> > +                          ConvertPixelFormatToFOURCC(graphicBuffer->getPixelFormat()));
> > +
> > + 
> 
> space

Will update in a next patch.

> 
> @@ +746,5 @@
> > +    data.mYSize = IntSize(dstWidth, dstHeight);
> > +    data.mYStride = dstWidth * lumaBpp / 8;
> > +    data.mCbCrStride = dstWidth * chromaBpp / 8;
> > +    data.mCbChannel = dstPtr + dstHeight * data.mYStride;
> > +    data.mCrChannel = data.mCbChannel +( dstHeight * data.mCbCrStride / 2);
> 
> spaces: " + (dstHeight"
> Also, it's misleading.  Should be (assuming asserts are used):
> data.mCbChannel + data.mCbCrStride * (dstHeight/2);

Will update in a next patch.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle out of gralloc

Apply the comment. Carry "r=jesup".

Comment 7

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce0671489ad

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ce0671489ad