Closed Bug 1113769 Opened 5 years ago Closed 5 years ago

Handle out of gralloc in MediaEngineGonkVideoSource::RotateImage()

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sotaro, Assigned: sotaro)

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
Assignee: nobody → sotaro.ikeda.g
Summary: Loop client does not render with Hardware accelerated composer (HWC) → Handle out of gralloc in MediaEngineGonkVideoSource::RotateImage()
Attached patch patch - Handle out of gralloc (obsolete) — Splinter Review
Attached patch patch - Handle out of gralloc (obsolete) — Splinter Review
Attachment #8539529 - Attachment is obsolete: true
Attachment #8539531 - Flags: review?(rjesup)
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);
Attachment #8539531 - Flags: review?(rjesup) → review+
(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.
Apply the comment. Carry "r=jesup".
Attachment #8539531 - Attachment is obsolete: true
Attachment #8540147 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ce0671489ad
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.