Closed Bug 1280839 Opened 4 years ago Closed 3 years ago

Recycle TextureClient in SharedPlanarYCbCrImage

Categories

(Core :: Graphics: Layers, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 8 obsolete files)

5.78 KB, patch
nical
: review+
nical
: feedback+
Details | Diff | Splinter Review
2.63 KB, patch
nical
: review+
nical
: feedback+
Details | Diff | Splinter Review
13.81 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.30 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
It is nice to recycle video buffer of FFmpegVideoDecoder.
Assignee: nobody → sotaro.ikeda.g
Version: 49 Branch → 50 Branch
ImageContainer::CreatePlanarYCbCrImage() creates SharedPlanarYCbCrImage. Then it is nice if video buffers are recycled is handled under SharedPlanarYCbCrImage.
Attachment #8763726 - Attachment description: patch - Update SendRecycleTexture usage → patch part 1 - Update SendRecycleTexture usage
Attachment #8763726 - Attachment is obsolete: true
Attachment #8763751 - Attachment is obsolete: true
Attachment #8763727 - Attachment is obsolete: true
Attachment #8764476 - Attachment description: Handle race condition in CompositableClient::GetTextureClientRecycler() → patch part 2 - Handle race condition in CompositableClient::GetTextureClientRecycler()
Attachment #8764477 - Attachment is obsolete: true
Comment on attachment 8763823 [details] [diff] [review]
patch part 1 - Update SendRecycleTexture usage

:nical, can you feedback to the patches?
Attachment #8763823 - Flags: feedback?(nical.bugzilla)
Attachment #8764476 - Flags: feedback?(nical.bugzilla)
Attachment #8764494 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8764494 [details] [diff] [review]
patch part 3 - Recycle TextureClient in SharedPlanarYCbCrImage

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

::: gfx/layers/BufferTexture.cpp
@@ +226,5 @@
>    return ImageDataSerializer::SizeFromBufferDescriptor(mDescriptor);
>  }
>  
> +gfx::IntSize
> +BufferTextureData::GetCbCrSize() const

Nit: this could return a Maybe<gfx::IntSize> (not very important).

::: gfx/layers/client/TextureClientRecycleAllocator.cpp
@@ +88,5 @@
> +YCbCrTextureClientAllocationHelper::IsCompatible(TextureClient* aTextureClient)
> +{
> +  MOZ_ASSERT(aTextureClient->GetFormat() == gfx::SurfaceFormat::YUV);
> +
> +  BufferTextureData* bufferData = aTextureClient->GetInternalData()->AsBufferTextureData();

Ideally we shouldn't be poking at the internal data, but I guess we don't have a good API to do what you want to do here so it will be ok for now.
Attachment #8764494 - Flags: feedback?(nical.bugzilla) → feedback+
Comment on attachment 8764476 [details] [diff] [review]
patch part 2 - Handle race condition in CompositableClient::GetTextureClientRecycler()

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

::: gfx/layers/client/CompositableClient.cpp
@@ +276,5 @@
> +  ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(runnable.forget());
> +
> +  // should stop the thread until done.
> +  while (!done) {
> +    barrier.Wait();

Hopefully we don't run into this case too often...
Attachment #8764476 - Flags: feedback?(nical.bugzilla) → feedback+
Attachment #8763823 - Flags: feedback?(nical.bugzilla) → feedback+
> 
> ::: gfx/layers/client/TextureClientRecycleAllocator.cpp
> @@ +88,5 @@
> > +YCbCrTextureClientAllocationHelper::IsCompatible(TextureClient* aTextureClient)
> > +{
> > +  MOZ_ASSERT(aTextureClient->GetFormat() == gfx::SurfaceFormat::YUV);
> > +
> > +  BufferTextureData* bufferData = aTextureClient->GetInternalData()->AsBufferTextureData();
> 
> Ideally we shouldn't be poking at the internal data, but I guess we don't
> have a good API to do what you want to do here so it will be ok for now.

Yea, we do not have a good API for now, then I used a bit dirty way.
Depends on: 1282341
Apply the comment.
Attachment #8764494 - Attachment is obsolete: true
Attachment #8763823 - Flags: review?(nical.bugzilla)
Attachment #8764476 - Flags: review?(nical.bugzilla)
Attachment #8765331 - Flags: review?(nical.bugzilla)
Attachment #8766182 - Flags: review?(nical.bugzilla)
Attachment #8766182 - Flags: review?(nical.bugzilla)
Attachment #8763823 - Flags: review?(nical.bugzilla) → review+
Attachment #8764476 - Flags: review?(nical.bugzilla) → review+
Attachment #8765331 - Flags: review?(nical.bugzilla) → review+
Attachment #8766182 - Attachment is obsolete: true
Attachment #8766211 - Flags: review?(nical.bugzilla)
Attachment #8766211 - Flags: review?(nical.bugzilla) → review+
Rebased.
Attachment #8765331 - Attachment is obsolete: true
Attachment #8766607 - Flags: review+
Rebased.
Attachment #8766211 - Attachment is obsolete: true
Attachment #8766610 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/00203ba4e8bf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Summary: Recycle video buffer of FFmpegVideoDecoder → Recycle TextureClient in SharedPlanarYCbCrImage
Duplicate of this bug: 1254006
You need to log in before you can comment on or make changes to this bug.