Closed Bug 1168015 Opened 5 years ago Closed 5 years ago

(LayerScope) Texture dump performance improvement on B2G

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

Take advantage of gralloc image, we don't need to create a FBO, render onto it, read it back from GPU memory, which is really slow.
Rendering performance get better after apply this patch. Looks like a right way to go
Attachment #8610198 - Attachment is obsolete: true
Attachment #8610407 - Flags: feedback?(hshih)
Attachment #8610201 - Attachment is obsolete: true
Attachment #8610407 - Attachment is obsolete: true
Attachment #8610407 - Flags: feedback?(hshih)
Attachment #8610506 - Flags: feedback?(hshih)
Next step:
1. bring concept of bug 1133007 to here.
   The solution in bug 1133007 is actually not work on B2G.
   That's because EGLImageTextureSource is derived from TextureSource, instead of DataTextureSource.
   Morris suggest move serial number from DatTextureSource to TextureSource, so that EGLImageTextureSource can leverage this counter to prevent sending the same texture repeatly.
2. texture swizzling on viewer size to render BGRA8888 graphic buffer correctly.
Comment on attachment 8610506 [details] [diff] [review]
[WIP] Bring LayerRenderState into TexturedEffect

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

::: gfx/layers/LayerScope.cpp
@@ +303,5 @@
> +        //printf_stderr("CKU: pixelFormat = %d.\n\r", pixelFormat);
> +        // HAL_PIXEL_FORMAT_RGBA_8888          = 1,
> +        // HAL_PIXEL_FORMAT_RGBX_8888          = 2,
> +        uint8_t* grallocData;
> +        if (BAD_VALUE == mBuffer->lock(GRALLOC_USAGE_SW_READ_OFTEN,

GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_NEVER

@@ +308,5 @@
> +                                       reinterpret_cast<void**>(&grallocData))) {
> +            return false;
> +        }
> +
> +        int32_t stride = mBuffer->getStride() * 4;

why we need to multiply 4 for stride?
Attachment #8610506 - Flags: feedback?(hshih) → feedback+
Attachment #8610506 - Attachment is obsolete: true
Comment on attachment 8610647 [details] [diff] [review]
Bring LayerRenderState into TexturedEffect

Hi Dan/ Jerry,
Please help to review this patch.

Original, to gather texture image, LayerScope draws texture onto a FBO and then read it back, which is really slow on smartphone. In this patch, I acquired source image via graphic buffer directly in TextureHost, much faster comparing to original approach.

The reason that I bring LayerRenderState into TexturedEffect is I need LayerRenderState::mSurface, which is a graphic buffer.
Attachment #8610647 - Attachment description: [WIP 2] Bring LayerRenderState into TexturedEffect → Bring LayerRenderState into TexturedEffect
Attachment #8610647 - Flags: review?(hshih)
Attachment #8610647 - Flags: review?(dglastonbury)
Rebase master
Attachment #8610647 - Attachment is obsolete: true
Attachment #8610647 - Flags: review?(hshih)
Attachment #8610647 - Flags: review?(dglastonbury)
Attachment #8611033 - Flags: review?(hshih)
Attachment #8611033 - Flags: review?(dglastonbury)
Comment on attachment 8611033 [details] [diff] [review]
Bring LayerRenderState into TexturedEffect

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

::: gfx/layers/LayerScope.cpp
@@ +295,5 @@
> +        tp->set_layerref(mLayerRef);
> +
> +        int format = mBuffer->getPixelFormat();
> +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {

how about RGB_888 and RGB_565?

@@ +306,5 @@
> +                                       reinterpret_cast<void**>(&grallocData))) {
> +            return false;
> +        }
> +
> +        int32_t stride = mBuffer->getStride() * 4;

Use bytesPerPixel()?
http://androidxref.com/5.1.0_r1/xref/frameworks/native/libs/ui/PixelFormat.cpp#24

@@ +319,5 @@
> +                                               sourceSize,
> +                                               compressedData.get());
> +
> +            if (compressedSize > 0) {
> +                tp->set_dataformat(LOCAL_GL_BGRA);

Please check LayerRenderState.FormatRBSwapped()
Attachment #8611033 - Flags: review?(hshih) → review+
Comment on attachment 8611033 [details] [diff] [review]
Bring LayerRenderState into TexturedEffect

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

::: gfx/layers/LayerScope.cpp
@@ +295,5 @@
> +        tp->set_layerref(mLayerRef);
> +
> +        int format = mBuffer->getPixelFormat();
> +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {

RGB_888 and RGB_565 fallback to original way: SendTextureSource, which will convert source image to RGBA format. 
At viewer side, we always need 4 channels format because of ImageData limitation.

@@ +306,5 @@
> +                                       reinterpret_cast<void**>(&grallocData))) {
> +            return false;
> +        }
> +
> +        int32_t stride = mBuffer->getStride() * 4;

OK

@@ +319,5 @@
> +                                               sourceSize,
> +                                               compressedData.get());
> +
> +            if (compressedSize > 0) {
> +                tp->set_dataformat(LOCAL_GL_BGRA);

OK, thanks.
Attachment #8611033 - Flags: review?(dglastonbury) → review+
(In reply to C.J. Ku[:cjku] from comment #12)
> Comment on attachment 8611033 [details] [diff] [review]
> Bring LayerRenderState into TexturedEffect
> 
> Review of attachment 8611033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +295,5 @@
> > +        tp->set_layerref(mLayerRef);
> > +
> > +        int format = mBuffer->getPixelFormat();
> > +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> > +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {
> 
> RGB_888 and RGB_565 fallback to original way: SendTextureSource, which will
> convert source image to RGBA format. 
I remember converting RGB565 into RGBA format is easy and I implemented it before on Leo device. If you also want to improve the performance for RGB565 format, you can check this: 
https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/row_common.cc#103
(In reply to Boris Chiou [:boris] from comment #13)
> (In reply to C.J. Ku[:cjku] from comment #12)
> > Comment on attachment 8611033 [details] [diff] [review]
> > Bring LayerRenderState into TexturedEffect
> > 
> > Review of attachment 8611033 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/LayerScope.cpp
> > @@ +295,5 @@
> > > +        tp->set_layerref(mLayerRef);
> > > +
> > > +        int format = mBuffer->getPixelFormat();
> > > +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> > > +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {
> > 
> > RGB_888 and RGB_565 fallback to original way: SendTextureSource, which will
> > convert source image to RGBA format. 
> I remember converting RGB565 into RGBA format is easy and I implemented it
> before on Leo device. If you also want to improve the performance for RGB565
> format, you can check this: 
> https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/
> row_common.cc#103
Well, in graphic buffers fetched from TextureHost, I noticed we only have RGBA8888 and RGBX8888. That's another reason why I don't want to work on 565 to 8888 conversion for now... I can't find use cases for it. But thanks for this information sharing :)
Attachment #8610604 - Flags: review?(boris.chiou)
Attachment #8611033 - Attachment is obsolete: true
Attachment #8611240 - Flags: review+
Comment on attachment 8610604 [details] [review]
handle RB swap at viewer

>https://github.com/mozilla/layerscope/pull/25
Attachment #8610604 - Attachment description: [WIP] handle RB swap at viewer → handle RB swap at viewer
(In reply to C.J. Ku[:cjku] from comment #16)
> Comment on attachment 8610604 [details] [review]
> handle RB swap at viewer
> 
> >https://github.com/mozilla/layerscope/pull/25

Could you please merge these two commits? Thanks.
(In reply to C.J. Ku[:cjku] from comment #14)
> (In reply to Boris Chiou [:boris] from comment #13)
> > (In reply to C.J. Ku[:cjku] from comment #12)
> > > Comment on attachment 8611033 [details] [diff] [review]
> > > Bring LayerRenderState into TexturedEffect
> > > 
> > > Review of attachment 8611033 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/layers/LayerScope.cpp
> > > @@ +295,5 @@
> > > > +        tp->set_layerref(mLayerRef);
> > > > +
> > > > +        int format = mBuffer->getPixelFormat();
> > > > +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> > > > +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {
> > > 
> > > RGB_888 and RGB_565 fallback to original way: SendTextureSource, which will
> > > convert source image to RGBA format. 
> > I remember converting RGB565 into RGBA format is easy and I implemented it
> > before on Leo device. If you also want to improve the performance for RGB565
> > format, you can check this: 
> > https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/
> > row_common.cc#103
> Well, in graphic buffers fetched from TextureHost, I noticed we only have
> RGBA8888 and RGBX8888. That's another reason why I don't want to work on 565
> to 8888 conversion for now... I can't find use cases for it. But thanks for
> this information sharing :)

Is it possible that any app uses a RGB565 image? I met this case while scrolling homescreen on Leo before. Maybe we don't use Leo any more, but just want to know the possibility.
Attachment #8610604 - Flags: review?(boris.chiou) → review+
Full try since touched a low-level gfx class - TexturedEffect..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd
Attachment #8611240 - Attachment is obsolete: true
Attachment #8611331 - Flags: review+
(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to C.J. Ku[:cjku] from comment #14)
> > (In reply to Boris Chiou [:boris] from comment #13)
> > > (In reply to C.J. Ku[:cjku] from comment #12)
> > > > Comment on attachment 8611033 [details] [diff] [review]
> > > > Bring LayerRenderState into TexturedEffect
> > > > 
> > > > Review of attachment 8611033 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > ::: gfx/layers/LayerScope.cpp
> > > > @@ +295,5 @@
> > > > > +        tp->set_layerref(mLayerRef);
> > > > > +
> > > > > +        int format = mBuffer->getPixelFormat();
> > > > > +        if (HAL_PIXEL_FORMAT_RGBA_8888 != format &&
> > > > > +            HAL_PIXEL_FORMAT_RGBX_8888 != format) {
> > > > 
> > > > RGB_888 and RGB_565 fallback to original way: SendTextureSource, which will
> > > > convert source image to RGBA format. 
> > > I remember converting RGB565 into RGBA format is easy and I implemented it
> > > before on Leo device. If you also want to improve the performance for RGB565
> > > format, you can check this: 
> > > https://dxr.mozilla.org/mozilla-central/source/media/libyuv/source/
> > > row_common.cc#103
> > Well, in graphic buffers fetched from TextureHost, I noticed we only have
> > RGBA8888 and RGBX8888. That's another reason why I don't want to work on 565
> > to 8888 conversion for now... I can't find use cases for it. But thanks for
> > this information sharing :)
> 
> Is it possible that any app uses a RGB565 image? I met this case while
> scrolling homescreen on Leo before. Maybe we don't use Leo any more, but
> just want to know the possibility.

OK. let's handle it in another bug
(In reply to C.J. Ku[:cjku] from comment #21)
> (In reply to Boris Chiou [:boris] from comment #18)
> > Is it possible that any app uses a RGB565 image? I met this case while
> > scrolling homescreen on Leo before. Maybe we don't use Leo any more, but
> > just want to know the possibility.
> 
> OK. let's handle it in another bug

Cool!
Keywords: checkin-needed
Hi, this patch failed to apply:

applying 0001-Bug-1168015-Dump-source-image-from-graphic-buffer-di.patch
patching file gfx/layers/composite/TiledContentHost.cpp
Hunk #1 FAILED at 456
1 out of 2 hunks FAILED -- saving rejects to file gfx/layers/composite/TiledContentHost.cpp.rej
patching file gfx/layers/composite/TiledContentHost.h
Hunk #1 FAILED at 278
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/composite/TiledContentHost.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 0001-Bug-1168015-Dump-source-image-from-graphic-buffer-di.patch

could you take a look, thanks!
Flags: needinfo?(cku)
Keywords: checkin-needed
Attachment #8611331 - Attachment is obsolete: true
Flags: needinfo?(cku)
Attachment #8612186 - Flags: review+
Hi Carsten,
Looks like someone changed the function signature of TiledContentHost::RenderTile at the same time. 
Created a new patch on the latest code. Would you pick this patch again?
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/223975f99ca3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.