1168015 - (LayerScope) Texture dump performance improvement on B2G

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[u459114]

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.

Comment 1

[u459114]

Attachment: WIP - hacking to evaluate performance improvement

Rendering performance get better after apply this patch. Looks like a right way to go

Comment 2

[u459114]

Attachment: WIP - hacking to evaluate performance improvement

Comment 3

[u459114]

Attachment: WIP2 - hacking to evaluate performance improvement

Comment 4

[u459114]

Attachment: [WIP] Bring LayerRenderState into TexturedEffect

Comment 5

[u459114]

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 6

[u459114]

Attachment: handle RB swap at viewer

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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?

Comment 8

[u459114]

Attachment: Bring LayerRenderState into TexturedEffect

Comment 9

[u459114]

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.

Comment 10

[u459114]

Attachment: Bring LayerRenderState into TexturedEffect

Rebase master

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 12

[u459114]

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.

Comment 13

[Boris Chiou [:boris]]

(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

Comment 14

[u459114]

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

Comment 15

[u459114]

Attachment: Fetch image buffer from graphic buffer

Comment 16

[u459114]

Comment on attachment 8610604 [details] [review]
handle RB swap at viewer

>https://github.com/mozilla/layerscope/pull/25

Comment 17

[Boris Chiou [:boris]]

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

Comment 18

[Boris Chiou [:boris]]

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

Comment 19

[u459114]

Full try since touched a low-level gfx class - TexturedEffect..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd

Comment 20

[u459114]

Attachment: Fetch image buffer from graphic buffer

Comment 21

[u459114]

(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

Comment 22

[Boris Chiou [:boris]]

(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!

Comment 23

[Carsten Book [:Tomcat]]

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!

Comment 24

[u459114]

Attachment: Fetch image buffer from graphic buffer

Comment 25

[u459114]

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?

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/223975f99ca3

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/223975f99ca3