Closed Bug 946720 Opened 11 years ago Closed 10 years ago

Enable new textures at ContentClient/ContentHost classes on gonk

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30
Tracking Flags:
Tracking Status
b2g-v1.3T --- ?

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 26 obsolete files)

patch_add_ipc_log
1.38 KB, patch
Details | Diff | Splinter Review
patch v24 - Enable new textures at ContentClient/ContentHost classes on gonk (r=nical)
19.86 KB, patch
sotaro
: review+
Details | Diff | Splinter Review 
+++ 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.
No longer blocks: 941735
blocking-b2g: --- → 1.3?
I locally enabled a new textures at ContentClient/ContentHost on gonk. I seems partially works, but causing a lot of genlock failures.
Assignee: nobody → sotaro.ikeda.g
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.
I pushed the wrong patch. This is correct one.
Attachment #8343732 - Attachment is obsolete: true
Remove Bug 908196 from dependent bug. The bug could  be independent.
No longer depends on: 908196
Fix some crashes.
Attachment #8343733 - Attachment is obsolete: true
Add GrallocTextureClientOGL::GetAsSurface() not to use serializer.
Attachment #8343884 - Attachment is obsolete: true
Serialize did hard in some places. gralloc buffer and serializer is incompatible.
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
Do not use gralloc at CanvasClient2D.
Attachment #8343924 - Attachment is obsolete: true
(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.
Thanks for the info! Yeah, it seems easier.
Apply Comment 10. Need more polish, but seems to work.
Attachment #8344016 - Attachment is obsolete: true
Blocks: 950079
No longer blocks: 950079
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.
Fix nits and re-enable gralloc.
Attachment #8344027 - Attachment is obsolete: true
No longer blocks: 925444
It seems better not to uplift this bug to b2g v1.3 only for fence handling. Clear "1.3?" flag.
blocking-b2g: 1.3? → ---
blocking-b2g: --- → 1.4?
Status: NEW → ASSIGNED
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.
Attachment #8347675 - Attachment is obsolete: true
The symptom is similar to Bug 784244.
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.
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.
Attachment #8357354 - Attachment is obsolete: true
attachment 8358544 [details] [diff] [review] still have a following problem.
 - Sometime other texture's content is drawn to some texutre area.
(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
Blocks: 957323
Comment 21 was incorrect. From the log, genlock failure was happened. Somehow, hide hold genlock when client side try to get write lock.
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.
Attachment #8358544 - Attachment is obsolete: true
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.
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.
Depends on: 959171
(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.
Fix genlock failure. nexus-4's boot animation drawing problem is still present.
Attachment #8358644 - Attachment is obsolete: true
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
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.
Attachment #8359371 - Attachment is obsolete: true
(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.
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.
Attachment #8359449 - Attachment is obsolete: true
Attached file stacktrace of the crash (obsolete) — 
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.
Attachment #8360013 - Attachment is patch: false
It becomes clear that attachment 8360003 [details] [diff] [review] makes Bug 958058's symptom worse. At first Bug 958058 need to be fixed.
Depends on: 958058
Depends on: 951732
No longer depends on: 958058
(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.
No longer depends on: 951732
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
Attachment #8360003 - Attachment is obsolete: true
Attachment #8361447 - Flags: review-
Attachment #8361447 - Flags: review-
Attached file logcat around pipe error (obsolete) — 
By Enabling IPC log and adding manual log, I saw a same log out pattern around pipe error.
(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.
I am going to confirm if the pipe problem is fixed on nexus-4.
(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.
All major problem seems to be fixed.
The patch is used to add ipc log to logcat.
(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.
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.
Flags: needinfo?(bent.mozilla)
(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.
Flags: needinfo?(bent.mozilla)
Blocks: 950050
Depends on: 952507
Check current patch's tryserver result.
https://tbpl.mozilla.org/?tree=Try&rev=f3d3cebb7843
(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.
(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.
Apply fix in Bug 959171.
Attachment #8361447 - Attachment is obsolete: true
Attachment #8362156 - Attachment is obsolete: true
Attachment #8360013 - Attachment is obsolete: true
(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.
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.
Unbitrot.
Attachment #8364568 - Attachment is obsolete: true
(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.
I just test the new ContentClient on Unagi with your patch + the patches from bug 952507 and it works! \o/
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?
(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!
(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.
Use PIXEL_FORMAT_RGBA_8888 with RB swap as PIXEL_FORMAT_BGRA_8888.
Attachment #8366726 - Attachment is obsolete: true
Sadly, with those updated version of the patch applied on Gecko master, bug 950050 is not starting to get fixed anymore :(
(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> https://tbpl.mozilla.org/?tree=Try&rev=d97eddb9e419

tryserver's test seems good.
Add gralloc buffer support check.
Attachment #8366781 - Attachment is obsolete: true
Attachment #8368681 - Flags: review?(nical.bugzilla)
No longer blocks: 950050
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.
Attachment #8368681 - Flags: review?(nical.bugzilla)
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.
Depends on: 966446
Attachment #8368681 - Flags: review?(nical.bugzilla)
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.
(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 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
Attachment #8368681 - Flags: review?(nical.bugzilla) → review-
(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?
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Thanks for the quick answer.
(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.
(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.
(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.
Apply comment except change to CreateTextureClientForDrawing().
Attachment #8368681 - Attachment is obsolete: true
un-bitrot.
Attachment #8370780 - Attachment is obsolete: true
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)
Status: ASSIGNED → NEW
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(sotaro.ikeda.g)
Depends on: 968872
Include Bug 968872's change for ease.
Attachment #8361702 - Attachment is obsolete: true
Attachment #8373740 - Attachment is obsolete: true
When CreateTextureClientForDrawing() is enabled in ImageClientSingle::UpdateImage(). At first, it is just because of single buffer. But the genlock happens even when ImageClientBuffered::UpdateImage().
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;
>  }
(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.
Flags: needinfo?(nical.bugzilla)
(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.
> 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.
(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.
- Fix genlock failure problem caused by ImageClient.
- In sync ImageLayer case, use ImageClientBuffered instead of ImageClientSingle on gonk.
Attachment #8374303 - Attachment is obsolete: true
Fix build failure on b2g emulator debug.
Attachment #8374904 - Attachment is obsolete: true
Attachment #8374994 - Flags: review?(nical.bugzilla)
un-bitrot
Attachment #8374994 - Attachment is obsolete: true
Attachment #8374994 - Flags: review?(nical.bugzilla)
Attachment #8375279 - Attachment is patch: true
Attachment #8375279 - Attachment mime type: text/x-patch → text/plain
Attachment #8375279 - Flags: review?(nical.bugzilla)
Attachment #8375279 - Flags: review?(nical.bugzilla) → review+
Committable patch. Carry 'r=nical'.
Attachment #8375279 - Attachment is obsolete: true
Attachment #8375477 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4e92a914e9c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Nical, By the fix, b2g does not use deprecated texture anymore. I confirmed it from source and by using actual phone.
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(nical.bugzilla)
blocking-b2g: 1.4? → ---
Depends on: 977227
No longer depends on: 977227
Attachment #8362156 - Attachment is obsolete: false
Depends on: 983971
Depends on: 986094
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: