946720 - Enable new textures at ContentClient/ContentHost classes on gonk

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

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

Bug 893301 is handling to enable new textures at ContentClient/ContentHost. But it has a problem to use gralloc buffers on gonk. This bug is going to handle the problem.

Comment 1

[Sotaro Ikeda [:sotaro]]

I locally enabled a new textures at ContentClient/ContentHost on gonk. I seems partially works, but causing a lot of genlock failures.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Enable new textures at ContentClient/ContentHost classes on gonk

The patch partially works, but has the following problems.
- lock screens animation does not drawn
- sometimes genlock failure happens.
- sometimes use after free of GraphcBuffer happens.
- Crash during WebM video playback.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Enable new textures at ContentClient/ContentHost classes on gonk

I pushed the wrong patch. This is correct one.

Comment 4

[Sotaro Ikeda [:sotaro]]

Remove Bug 908196 from dependent bug. The bug could  be independent.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v2 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix some crashes.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v3 - Enable new textures at ContentClient/ContentHost classes on gonk

Add GrallocTextureClientOGL::GetAsSurface() not to use serializer.

Comment 7

[Sotaro Ikeda [:sotaro]]

Serialize did hard in some places. gralloc buffer and serializer is incompatible.

Comment 8

[Sotaro Ikeda [:sotaro]]

A lot of genlock was caused by non-GL canvas layer. CanvasClient2D uses only one buffer. By ths patch, CompositableClient::CreateBufferTextureClient() allocates gralloc buffer. The function is used also by CanvasClient2D. Gralloc buffer can not be written during it is rendered. Therefore, a lot of genlock failure was happen.
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp#102

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v4 - Enable new textures at ContentClient/ContentHost classes on gonk

Do not use gralloc at CanvasClient2D.

Comment 10

[Nick Cameron [:nrc]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> A lot of genlock was caused by non-GL canvas layer. CanvasClient2D uses only
> one buffer. By ths patch, CompositableClient::CreateBufferTextureClient()
> allocates gralloc buffer. The function is used also by CanvasClient2D.
> Gralloc buffer can not be written during it is rendered. Therefore, a lot of
> genlock failure was happen.
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.
> cpp#102

It might be easier to get this working only for content, not for canvas and image layers by modifying CreateTextureClientForDrawing rather than CreateBufferTextureClient. That was Nical's plan for doing this.

Comment 11

[Sotaro Ikeda [:sotaro]]

Thanks for the info! Yeah, it seems easier.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v5 - Enable new textures at ContentClient/ContentHost classes on gonk

Apply Comment 10. Need more polish, but seems to work.

Comment 13

[Sotaro Ikeda [:sotaro]]

I thought attachment 8344027 [details] [diff] [review] fixed the problem. But it is not. The patch was incorrect. It unintentionally disables the gralloc buffer :-( After re-enabling the gralloc buffer. I recognize the some rendering problem is still present.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v6 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix nits and re-enable gralloc.

Comment 15

[Sotaro Ikeda [:sotaro]]

It seems better not to uplift this bug to b2g v1.3 only for fence handling. Clear "1.3?" flag.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v7 - Enable new textures at ContentClient/ContentHost classes on gonk

unbitrot the path. By the fix around d3d new textures(Bug 938591). Rendering result seems better. But some times I can see snow artifact on the bottom of the homescreen. It typically happens when app try to render to gralloc buffers without genclock.

Comment 17

[Sotaro Ikeda [:sotaro]]

The symptom is similar to Bug 784244.

Comment 18

[Sotaro Ikeda [:sotaro]]

At first, I thought the artifact is caused by client side as in Bug 784244. But I am going to think that it is a Host side's problem.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v8 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix snow artifact. The artifact was injected by me from patch v6 to patch v7 :-( In GrallocTextureClientOGL, USAGE_SW_WRITE_OFTEN flag is no passed correctly to gralloc buffer... And fix nits.

Comment 20

[Sotaro Ikeda [:sotaro]]

attachment 8358544 [details] [diff] [review] still have a following problem.
 - Sometime other texture's content is drawn to some texutre area.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> attachment 8358544 [details] [diff] [review] still have a following problem.
>  - Sometime other texture's content is drawn to some texutre area.

Somehow, there are cases that EGL image becomes stale. When I change a code to re-create EGL image in each buffer swap in SharedTextureSourceOGL, the rendering problems are almost gone on hamachi.

GrallocTextureSourceOGL::SetCompositableBackendSpecificData()
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#172

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment 21 was incorrect. From the log, genlock failure was happened. Somehow, hide hold genlock when client side try to get write lock.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v9 - Enable new textures at ContentClient/ContentHost classes on gonk

Add calling gl()->fEGLImageTargetTexture2D() when binding texture. Current GrallocTextureSourceOGL::BindTexture() does not call fEGLImageTargetTexture2D(). Without calling the function, EGLImage bounded to texture is not updated. By this, thebes layer's back buffer was continues to bounded to texture. Then, genlock failure happened.

Until v1.2, GrallocTextureSourceOGL's usage was limited only to Image Layer. Image layer uses new GrallocTextureHostOGL for each rendering update, therefore the problem did not happen.

Comment 24

[Sotaro Ikeda [:sotaro]]

Around GrallocTextureHostOGL, I found 2 problem about how to use OpenGL. It seems to split to a new bug. It could affect to other layer.

Comment 25

[Sotaro Ikeda [:sotaro]]

By attachment 8358644 [details] [diff] [review], on hamachi, all problems seems to be fixed. But I still see a problem on nexus-4's boot log's drawing.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> Around GrallocTextureHostOGL, I found 2 problem about how to use OpenGL. It
> seems to split to a new bug. It could affect to other layer.

Bug 959171 is created for it.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v10 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix genlock failure. nexus-4's boot animation drawing problem is still present.

Comment 28

[Sotaro Ikeda [:sotaro]]

nexus-4's rendering problem happened at Image layer. From Comment 10, I thought CreateTextureClientForDrawing() is not used for image layer. But it is used in ImageClientSingle::UpdateImage(). Somehow ImageClientSingle::UpdateImage() is not rendered correctly when gralloc is allocated here. Bottom part of the Image is rendered as black. From Comment 10, it seems better to call CreateBufferTextureClient() here.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp#239

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v11 - Enable new textures at ContentClient/ContentHost classes on gonk

Use CreateBufferTextureClient() in ImageClientSingle::UpdateImage(). By the patch, big drawing problem in nexus-4 is fixed. But there is still one drawing problem. During contact app scrolling, header part of the app sometimes flush as white.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> nexus-4's rendering problem happened at Image layer. From Comment 10, I
> thought CreateTextureClientForDrawing() is not used for image layer. But it
> is used in ImageClientSingle::UpdateImage(). Somehow
> ImageClientSingle::UpdateImage() is not rendered correctly when gralloc is
> allocated here. Bottom part of the Image is rendered as black.

I do not understand the reason of the image rendering problem. In this bug, I am going to continue to use same buffer as current gecko uses. DeprecatedImageClientSingle::UpdateImage() uses shmem here.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v12 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix the white flash drawing problem in Comment 29. It is caused by 

- By TextureClient's destruction, GrallocTextureHostOGL::ForgetSharedData() is called. Then the ForgetSharedData() calls GrallocTextureSourceOGL::ForgetBuffer().
By it, GrallocTextureSourceOGL is not drawin after the function call. It caused the white flash drawing.

GrallocTextureSourceOGL needs to be valid even after GrallocTextureHostOGL destruction in current architecture.

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: stacktrace of the crash

When enabling the new texture, I often saw the crash especially on nexus-4. On hamachi, the crash is not so often. I am not sure the crash is caused by the patch or just trigger it.

Comment 33

[Sotaro Ikeda [:sotaro]]

It becomes clear that attachment 8360003 [details] [diff] [review] makes Bug 958058's symptom worse. At first Bug 958058 need to be fixed.

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> It becomes clear that attachment 8360003 [details] [diff] [review] makes Bug
> 958058's symptom worse. At first Bug 958058 need to be fixed.

Bug 951732 is the correct bug. Bug 958058 is already duped to it.

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v13 - Enable new textures at ContentClient/ContentHost classes on gonk

Rendering problem was fixed. But ipc pipe error sometimes happens.

> [Parent 134] WARNING: pipe error (153): Connection reset by peer: file /home/sotaro/b2g_master_hamachi/B2G/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 446

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: logcat around pipe error

By Enabling IPC log and adding manual log, I saw a same log out pattern around pipe error.

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> Rendering problem was fixed. But ipc pipe error sometimes happens.
> 
> > [Parent 134] WARNING: pipe error (153): Connection reset by peer: file /home/sotaro/b2g_master_hamachi/B2G/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 446

By debugging a long time. It become clear that the problem was Bug 959842 :-( I already tested the fix and problem was not fixed. So, I thought the pipe error is different from Bug 959842. By Bug 959842, an application's main thread was killed. But the crash does not happen. b2g and application continues processing. Since the main thread crash, the application's MessageChannel::OnMaybeDequeueOne() does not called. By it, IPC communication between b2g and application stop. But both continue to send IPC messages until error happens.

Comment 38

[Sotaro Ikeda [:sotaro]]

I am going to confirm if the pipe problem is fixed on nexus-4.

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> I am going to confirm if the pipe problem is fixed on nexus-4.

On nexus-4, the pipe problem was also fixed.

Comment 40

[Sotaro Ikeda [:sotaro]]

All major problem seems to be fixed.

Comment 41

[Sotaro Ikeda [:sotaro]]

Attachment: patch_add_ipc_log

The patch is used to add ipc log to logcat.

Comment 42

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> Created attachment 8362156 [details] [diff] [review]
> patch_add_ipc_log
> 
> The patch is used to add ipc log to logcat.

I am not sure why printf_stderr() is not used for ipc logout.

Comment 43

[Sotaro Ikeda [:sotaro]]

bent, is there a reason not to use printf_stderr() for ipc log? For ease of debugging on b2g, printf_stderr() seems better, I think.

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Sotaro Ikeda [:sotaro] from comment #43)
> bent, is there a reason not to use printf_stderr() for ipc log? For ease of
> debugging on b2g, printf_stderr() seems better, I think.

Ted is working on this in bug 879580.

Comment 45

[Sotaro Ikeda [:sotaro]]

Check current patch's tryserver result.
https://tbpl.mozilla.org/?tree=Try&rev=f3d3cebb7843

Comment 46

[Sotaro Ikeda [:sotaro]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #43)
> > bent, is there a reason not to use printf_stderr() for ipc log? For ease of
> > debugging on b2g, printf_stderr() seems better, I think.
> 
> Ted is working on this in bug 879580.

Thanks for the info.

Comment 47

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> Check current patch's tryserver result.
> https://tbpl.mozilla.org/?tree=Try&rev=f3d3cebb7843

Hmm, almost all b2g emulator test failed.

Comment 48

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v14 - Enable new textures at ContentClient/ContentHost classes on gonk

Apply fix in Bug 959171.

Comment 49

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> Check current patch's tryserver result.
> https://tbpl.mozilla.org/?tree=Try&rev=f3d3cebb7843

Test failure reason is the following.
 - emulator does not support HAL_PIXEL_FORMAT_RGBX_8888.
 - After creating GrallocTextureClient, it failed to allocate gralloc buffer.
   But it can not fallback to Shemen.

In deprecated Texture, DeprecatedTextureClientShmem is used for gralloc buffer allocation. deprecated texture uses SurfaceDescriptor. It is memory type independent. So, it was possible to support gralloc buffer by DeprecatedTextureClientShmem.

Comment 50

[Sotaro Ikeda [:sotaro]]

Fallback to shmem was caused by incorrect flag setting of gralloc buffer creation. When the both of the following flag is set for gralloc buffer creation, emulator's gralloc hal failed to allocate gralloc buffers because of invalid argument.
 - android::GraphicBuffer::USAGE_SW_WRITE_OFTEN 
 - android::GraphicBuffer::USAGE_HW_RENDER 

Anyway, Current new texture client does not support fallback from gralloc to shmem. It seems necessary.

Comment 51

[Sotaro Ikeda [:sotaro]]

Attachment: patch v15 - Enable new textures at ContentClient/ContentHost classes on gonk

Unbitrot.

Comment 52

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #51)
> Created attachment 8366726 [details] [diff] [review]
> patch v15 - Enable new textures at ContentClient/ContentHost classes on gonk
> 
> Unbitrot.

The patch still have a problem of rb swap on emulator.

Comment 53

[Nicolas Silva [:nical]]

I just test the new ContentClient on Unagi with your patch + the patches from bug 952507 and it works! \o/

Comment 54

[Nicolas Silva [:nical]]

Comment on attachment 8366726 [details] [diff] [review]
patch v15 - Enable new textures at ContentClient/ContentHost classes on gonk

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

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ -220,3 @@
>      break;
>    case gfx::SurfaceFormat::B8G8R8A8:
> -    format = swapRB ? android::PIXEL_FORMAT_RGBA_8888 : android::PIXEL_FORMAT_BGRA_8888;

Could the emulator rb swap problem be related to this change?

Comment 55

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #53)
> I just test the new ContentClient on Unagi with your patch + the patches
> from bug 952507 and it works! \o/

Good!

Comment 56

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #54)
> 
> ::: gfx/layers/opengl/GrallocTextureClient.cpp
> @@ -220,3 @@
> >      break;
> >    case gfx::SurfaceFormat::B8G8R8A8:
> > -    format = swapRB ? android::PIXEL_FORMAT_RGBA_8888 : android::PIXEL_FORMAT_BGRA_8888;
> 
> Could the emulator rb swap problem be related to this change?

rb swap flag was added by Bug 843599. But it is not consistent way. So, I changed it. I just confirmed that PIXEL_FORMAT_BGRA_8888 is allocated by emulator's gralloc hal, but it is not rendered correctly On OpenGL.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch v16 - Enable new textures at ContentClient/ContentHost classes on gonk

Use PIXEL_FORMAT_RGBA_8888 with RB swap as PIXEL_FORMAT_BGRA_8888.

Comment 58

[:gerard-majax]

Sadly, with those updated version of the patch applied on Gecko master, bug 950050 is not starting to get fixed anymore :(

Comment 59

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=d97eddb9e419

Comment 60

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> https://tbpl.mozilla.org/?tree=Try&rev=d97eddb9e419

tryserver's test seems good.

Comment 61

[Sotaro Ikeda [:sotaro]]

Attachment: patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk

Add gralloc buffer support check.

Comment 62

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8368681 [details] [diff] [review]
patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk

Current patch seems to have a crash problem. I am going to investigate about it.

Comment 63

[Sotaro Ikeda [:sotaro]]

I confirmed that the app's crash in Comment 62 happens on default master hamachi. And by applying the patch, the another problem(use after free) as same as bug 862324 additionally happens on new texture.

Comment 64

[Nicolas Silva [:nical]]

Comment on attachment 8368681 [details] [diff] [review]
patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk

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

::: gfx/layers/client/ImageClient.cpp
@@ +236,5 @@
>      if (!mFrontBuffer) {
>        gfxImageFormat format
>          = gfxPlatform::GetPlatform()->OptimalFormatForContent(surface->GetContentType());
> +      mFrontBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),
> +                                               mTextureFlags);

This will regress windows OMTC by making it use shmem instead of D3D textures. Why not use CreateTextureClientForDrawing here?

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +211,5 @@
> +  if (aMode & OPEN_READ) {
> +    usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> +  }
> +  if (aMode & OPEN_WRITE) {
> +    usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;

That's nice!

@@ +266,5 @@
> +  Lock(OPEN_READ_WRITE);
> +
> +  gfxImageFormat format = ImageFormatForPixelFormat(mGraphicBuffer->getPixelFormat());
> +  long pixelStride = mGraphicBuffer->getStride();
> +  long byteStride = pixelStride * gfxASurface::BytePerPixelFromFormat(format);

Use gfx::BytesPerPixel(gfx::SurfaceFromat) defined in 2d/Tools.h instead of the thebes version.

@@ +267,5 @@
> +
> +  gfxImageFormat format = ImageFormatForPixelFormat(mGraphicBuffer->getPixelFormat());
> +  long pixelStride = mGraphicBuffer->getStride();
> +  long byteStride = pixelStride * gfxASurface::BytePerPixelFromFormat(format);
> +  return gfxPlatform::GetPlatform()->CreateDrawTargetForData(GetBuffer(),

With GetAsDrawTarget you need to store the DrawTarget as a member of GrallocTextureClient, similarly to what TextureClientD3D11 does, because the texture client is supposed to flush the DrawTarget in Unlock and because several calls to GetAsDrawTarget are expected to return the same DrawTarget. Another advantage of doing that is that DrawTarget are slightly expensive to create/destroy, so keeping it around for next frame helps us keep cached stuff.

@@ +277,5 @@
> +already_AddRefed<gfxASurface>
> +GrallocTextureClientOGL::GetAsSurface()
> +{
> +  MOZ_ASSERT(IsValid());
> +  Lock(OPEN_READ_WRITE);

Please remove the lock here and assert that the texture is locked instead. It is the responsibility of the caller to lock beforehand otherwise matching lock with unlock becomes a nightmare.

Comment 65

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #64)
> Comment on attachment 8368681 [details] [diff] [review]
> 
> With GetAsDrawTarget you need to store the DrawTarget as a member of
> GrallocTextureClient, similarly to what TextureClientD3D11 does, because the
> texture client is supposed to flush the DrawTarget in Unlock and because
> several calls to GetAsDrawTarget are expected to return the same DrawTarget.
> Another advantage of doing that is that DrawTarget are slightly expensive to
> create/destroy, so keeping it around for next frame helps us keep cached
> stuff.

Actually, you may want to null out the DrawTarget on unlock after the flush because I am not sure that locking the gralloc buffer again will give you the same pointer. If it does always give the same address then ignore what I just said.

Comment 66

[Nicolas Silva [:nical]]

Comment on attachment 8368681 [details] [diff] [review]
patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk

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

See comments above

Comment 67

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #64)
> Comment on attachment 8368681 [details] [diff] [review]
> patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk
> 
> Review of attachment 8368681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +236,5 @@
> >      if (!mFrontBuffer) {
> >        gfxImageFormat format
> >          = gfxPlatform::GetPlatform()->OptimalFormatForContent(surface->GetContentType());
> > +      mFrontBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),
> > +                                               mTextureFlags);
> 
> This will regress windows OMTC by making it use shmem instead of D3D
> textures. Why not use CreateTextureClientForDrawing here?

From Comment 10 by nrc, CreateTextureClientForDrawing seems not for ImageLayer, but only for content(thebes layer). How do you differentiate CreateTextureClientForDrawing and CreateBufferTextureClient?

Comment 68

[Nicolas Silva [:nical]]

CreateTextureClientForDrawing creates a TextureClient with the guarantee that you can draw into it with a DrawTarget, whereas CreateBufferTextureClient creates a TextureClient with the guarantee that you can map the texture in memory and get a pointer to its mapped buffer.

It turns out that BufferTextureClients can also give draw targets since you can create a DT around the mapped data (hence CreateBufferTextureClientForDrawing defaulting to CreateBufferTextureClient) but it won't give you the best option. For instance, a BufferTextureClient will be very very slow with direct 2D because the latter cannot create a draw target around a pointer to pixel data, so we have a slow fallback that does readbacks and uploads to/from the gpu.

Comment 69

[Sotaro Ikeda [:sotaro]]

Thanks for the quick answer.

Comment 70

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #64)
> Comment on attachment 8368681 [details] [diff] [review]
> patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk
> 
> Review of attachment 8368681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +236,5 @@
> >      if (!mFrontBuffer) {
> >        gfxImageFormat format
> >          = gfxPlatform::GetPlatform()->OptimalFormatForContent(surface->GetContentType());
> > +      mFrontBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),
> > +                                               mTextureFlags);
> 
> This will regress windows OMTC by making it use shmem instead of D3D
> textures. Why not use CreateTextureClientForDrawing here?

From Comment 10, I misunderstood the function's role. Will update in next patch.

> ::: gfx/layers/opengl/GrallocTextureClient.cpp
> @@ +211,5 @@
> > +  if (aMode & OPEN_READ) {
> > +    usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> > +  }
> > +  if (aMode & OPEN_WRITE) {
> > +    usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> 
> That's nice!
> 
> @@ +266,5 @@
> > +  Lock(OPEN_READ_WRITE);
> > +
> > +  gfxImageFormat format = ImageFormatForPixelFormat(mGraphicBuffer->getPixelFormat());
> > +  long pixelStride = mGraphicBuffer->getStride();
> > +  long byteStride = pixelStride * gfxASurface::BytePerPixelFromFormat(format);
> 
> Use gfx::BytesPerPixel(gfx::SurfaceFromat) defined in 2d/Tools.h instead of
> the thebes version.

Will fix in next patch.

> 
> @@ +267,5 @@
> > +
> > +  gfxImageFormat format = ImageFormatForPixelFormat(mGraphicBuffer->getPixelFormat());
> > +  long pixelStride = mGraphicBuffer->getStride();
> > +  long byteStride = pixelStride * gfxASurface::BytePerPixelFromFormat(format);
> > +  return gfxPlatform::GetPlatform()->CreateDrawTargetForData(GetBuffer(),
> 
> With GetAsDrawTarget you need to store the DrawTarget as a member of
> GrallocTextureClient, similarly to what TextureClientD3D11 does, because the
> texture client is supposed to flush the DrawTarget in Unlock and because
> several calls to GetAsDrawTarget are expected to return the same DrawTarget.
> Another advantage of doing that is that DrawTarget are slightly expensive to
> create/destroy, so keeping it around for next frame helps us keep cached
> stuff.

I did not recognize "several calls to GetAsDrawTarget are expected to return the same DrawTarget." part. I will update in a next patch.

> 
> @@ +277,5 @@
> > +already_AddRefed<gfxASurface>
> > +GrallocTextureClientOGL::GetAsSurface()
> > +{
> > +  MOZ_ASSERT(IsValid());
> > +  Lock(OPEN_READ_WRITE);
> 
> Please remove the lock here and assert that the texture is locked instead.
> It is the responsibility of the caller to lock beforehand otherwise matching
> lock with unlock becomes a nightmare.

I forgot to remove it. The past gecko requires it, the current code should not.

Comment 71

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> I did not recognize "several calls to GetAsDrawTarget are expected to return
> the same DrawTarget." part. I will update in a next patch.
> 

That's my bad for poorly documenting this. I'll add some doc.

Comment 72

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> (In reply to Nicolas Silva [:nical] from comment #64)
> > Comment on attachment 8368681 [details] [diff] [review]
> > patch v17 - Enable new textures at ContentClient/ContentHost classes on gonk
> > 
> > Review of attachment 8368681 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/client/ImageClient.cpp
> > @@ +236,5 @@
> > >      if (!mFrontBuffer) {
> > >        gfxImageFormat format
> > >          = gfxPlatform::GetPlatform()->OptimalFormatForContent(surface->GetContentType());
> > > +      mFrontBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),
> > > +                                               mTextureFlags);
> > 
> > This will regress windows OMTC by making it use shmem instead of D3D
> > textures. Why not use CreateTextureClientForDrawing here?
> 
> From Comment 10, I misunderstood the function's role. Will update in next
> patch.
> 

I forgot that the change cause the genlock failure on gonk. And I confirmed the change actually caused the genlock failure.

Comment 73

[Sotaro Ikeda [:sotaro]]

Attachment: patch v18 - Enable new textures at ContentClient/ContentHost classes on gonk

Apply comment except change to CreateTextureClientForDrawing().

Comment 74

[Sotaro Ikeda [:sotaro]]

Attachment: patch v19 - Enable new textures at ContentClient/ContentHost classes on gonk

un-bitrot.

Comment 75

[:gerard-majax]

Is this patch supposed to fix the unregisterBuffer() returning -EINVAL ? Because I retried applying it on Nexus S and disabling gralloc workarounds, and I still get those in logcat:
W/GraphicBufferMapper(  612): unregisterBuffer(0x44cd8e00) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x44cd8dc0) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x44d2a480) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x44d2a2c0) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x45221e80) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x45221bc0) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x45222140) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x45222180) failed -22 (Invalid argument)
W/GraphicBufferMapper(  612): unregisterBuffer(0x45222600) failed -22 (Invalid argument)

Comment 76

[Sotaro Ikeda [:sotaro]]

(In reply to Alexandre LISSY :gerard-majax from comment #75)
> Is this patch supposed to fix the unregisterBuffer() returning -EINVAL ?
> Because I retried applying it on Nexus S and disabling gralloc workarounds,
> and I still get those in logcat:

The patch does not intended to fix the nexus-s problem. The patch fix how to use gralloc buffers. I am going to think nexus-s's gralloc buffer's driver have some defect. It seems to request some pre-requisites to how to map gralloc buffers.

Comment 77

[Sotaro Ikeda [:sotaro]]

Attachment: patch v20 - Enable new textures at ContentClient/ContentHost classes on gonk

Include Bug 968872's change for ease.

Comment 78

[Sotaro Ikeda [:sotaro]]

When CreateTextureClientForDrawing() is enabled in ImageClientSingle::UpdateImage(). At first, it is just because of single buffer. But the genlock happens even when ImageClientBuffered::UpdateImage().

Comment 79

[Sotaro Ikeda [:sotaro]]

From the investigation, I understand why genlock happens even at ImageClientBuffered. There is no image update, buffer swap is skipped. Even in this case, front and buffer is swapped in client side. But in parent side, there is no buffer swap. So, in next rendering ImageClientBuffered's back buffer becomes, ImageHost's front buffer. Then the genlock happens because of genlock conflict between parent side and child side.

>  if (mLastPaintedImageSerial == image->GetSerial()) {
>    return true;
>  }

Comment 80

[Nicolas Silva [:nical]]

(In reply to Alexandre LISSY :gerard-majax from comment #75)
> Is this patch supposed to fix the unregisterBuffer() returning -EINVAL ?
> Because I retried applying it on Nexus S and disabling gralloc workarounds,
> and I still get those in logcat:

This patch is supposed to help get rid of old deprecated code. It is not intended to fix the unregisterBuffer problem directly.

Comment 81

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #79)
> From the investigation, I understand why genlock happens even at
> ImageClientBuffered. 

If we want to use gralloc buffer in sync ImageLayer case, we need to use ImageClientBuffered instead of ImageClientSingle. Otherwise, fall into genlock conflict between Host side and Child side when the sync ImageLayer's image data is updated.

Comment 82

[Sotaro Ikeda [:sotaro]]

> If we want to use gralloc buffer in sync ImageLayer case, we need to use
> ImageClientBuffered instead of ImageClientSingle. Otherwise, fall into
> genlock conflict between Host side and Child side when the sync ImageLayer's
> image data is updated.

Current implementation uses shmem for it.

Comment 83

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #82)
> > If we want to use gralloc buffer in sync ImageLayer case, we need to use
> > ImageClientBuffered instead of ImageClientSingle. Otherwise, fall into
> > genlock conflict between Host side and Child side when the sync ImageLayer's
> > image data is updated.
> 
> Current implementation uses shmem for it.

Then, ashem causes to block hw composer rendering.

Comment 84

[Sotaro Ikeda [:sotaro]]

Attachment: patch v21 - Enable new textures at ContentClient/ContentHost classes on gonk

- Fix genlock failure problem caused by ImageClient.
- In sync ImageLayer case, use ImageClientBuffered instead of ImageClientSingle on gonk.

Comment 85

[Sotaro Ikeda [:sotaro]]

Attachment: patch v22 - Enable new textures at ContentClient/ContentHost classes on gonk

Fix build failure on b2g emulator debug.

Comment 86

[Sotaro Ikeda [:sotaro]]

Attachment: patch v23 - Enable new textures at ContentClient/ContentHost classes on gonk

un-bitrot

Comment 87

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=dd6af6492913

Comment 88

[Sotaro Ikeda [:sotaro]]

Attachment: patch v24 - Enable new textures at ContentClient/ContentHost classes on gonk (r=nical)

Committable patch. Carry 'r=nical'.

Comment 89

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f4e92a914e9c

Comment 90

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4e92a914e9c

Comment 91

[Sotaro Ikeda [:sotaro]]

Nical, By the fix, b2g does not use deprecated texture anymore. I confirmed it from source and by using actual phone.