Use texture handle directly for video with webrender

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 9 obsolete attachments)

4.24 KB, patch
Details | Diff | Splinter Review
6.47 KB, patch
Details | Diff | Splinter Review
4.94 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.80 KB, patch
Details | Diff | Splinter Review
34.31 KB, patch
Details | Diff | Splinter Review
This bug try to pass the hardware video decoded texture handle to webrender.
Created attachment 8847018 [details] [diff] [review]
add WrExternalImage binding utility function.

MozReview-Commit-ID: G10ju70yGSS
Created attachment 8847019 [details] [diff] [review]
use WrImageDescriptor for wr_api_add_external_image_handle().

MozReview-Commit-ID: GQxQZDrqUR9
Created attachment 8847020 [details] [diff] [review]
create RenderBufferTextureHost and RenderIacIOSurfaceTextureHostOGL.

MozReview-Commit-ID: KjdLfYyBByf
Created attachment 8847021 [details] [diff] [review]
use texture handle directly with webrender.

MozReview-Commit-ID: 7TGxbwGn2fo
I haven't finished the shader updating in WR. So, we still can't use a hardware decoder for video.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(nical.bugzilla)
(Assignee)

Updated

a year ago
Depends on: 1348684
Created attachment 8851333 [details] [diff] [review]
add WrExternalImage binding utility function.

MozReview-Commit-ID: G10ju70yGSS
Attachment #8851333 - Flags: review?(sotaro.ikeda.g)
Attachment #8851333 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

a year ago
Attachment #8847018 - Attachment is obsolete: true
Attachment #8847019 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8847020 - Attachment is obsolete: true
Attachment #8847021 - Attachment is obsolete: true
Created attachment 8851334 [details] [diff] [review]
use WrImageDescriptor for wr_api_add_external_image_handle().

MozReview-Commit-ID: GQxQZDrqUR9
Attachment #8851334 - Flags: review?(sotaro.ikeda.g)
Attachment #8851334 - Flags: review?(nical.bugzilla)
Created attachment 8851335 [details] [diff] [review]
create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

1) make RenderTextureHost into a abstract base class for all RenderXXXTextureHost.
2) create a base class RenderTextureHostOGL for all texture handle base texture.
3) create RenderBufferTextureHost for buffer texture at render thread.
4) create RenderMacIOSurfaceTextureHostOGL for MacIOSurface at render thread.

MozReview-Commit-ID: 1t3NgXS8FIp
Attachment #8851335 - Flags: review?(sotaro.ikeda.g)
Attachment #8851335 - Flags: review?(nical.bugzilla)
Created attachment 8851336 [details] [diff] [review]
use texture handle directly with webrender.

MozReview-Commit-ID: KxesNcPWsFu
Attachment #8851336 - Flags: review?(sotaro.ikeda.g)
Attachment #8851336 - Flags: review?(nical.bugzilla)
Attachment #8851334 - Flags: review?(sotaro.ikeda.g) → review+
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8851333 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8851333 - Flags: review?(nical.bugzilla) → review+
Attachment #8851334 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8851336 [details] [diff] [review]
use texture handle directly with webrender.

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

::: gfx/webrender_bindings/WebRenderTypes.h
@@ +43,5 @@
>  
>  inline Maybe<WrImageFormat>
>  SurfaceFormatToWrImageFormat(gfx::SurfaceFormat aFormat) {
>    switch (aFormat) {
> +    case gfx::SurfaceFormat::R8G8B8X8:

Do we actually get RGBX images? WebRender doesn't have a RGB/BGR distinction (and it would be great if we never have to add this mess to WR).
Attachment #8851336 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8851335 [details] [diff] [review]
create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

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

::: gfx/layers/wr/WebRenderTextureHost.h
@@ +16,1 @@
>  class WebRenderTextureHost : public TextureHost

I know this class already exists, but could you add a comment here briefly explaining what WebRenderTextureHost represents and how it is used? (In this patch or in a followup if you prefer)

@@ +30,5 @@
>    virtual void Unlock() override;
>  
>    virtual gfx::SurfaceFormat GetFormat() const override;
>  
> +  virtual gfx::SurfaceFormat GetReadFormat() const override;

A comment explaining the difference between GetFormat and GetReadFormat would be useful.

@@ +50,5 @@
>    uint64_t GetExternalImageKey() { return mExternalImageId; }
>  
>    int32_t GetRGBStride();
> +
> +  bool WrapNativeHandle() { return mWrapNativeHandle; }

The name confuses me because it reads like this method does the work of wraping a native handle and returns true if it suceeded but it's in fact a simple getter. Could you rename it into "WrapsNativeHandle" or something along these lines?
Attachment #8851335 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8851335 [details] [diff] [review]
create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

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

::: gfx/layers/composite/TextureHost.cpp
@@ +194,5 @@
>  TextureHost::Create(const SurfaceDescriptor& aDesc,
>                      ISurfaceAllocator* aDeallocator,
>                      LayersBackend aBackend,
>                      TextureFlags aFlags)
>  {

WebRenderTextureHost supports only BufferTextureHost and MacIOSurfaceTextureHost, but TextureHost::Create seems to wrap all types of TextureHosts, it there a reason to do it?
Attachment #8851336 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8851335 [details] [diff] [review]
create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

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

Is it possible to create a patch by using |hg cp| for RenderBufferTextureHost like Bug 1316479 Comment 2?
(Assignee)

Updated

a year ago
Depends on: 1351187
Comment on attachment 8851336 [details] [diff] [review]
use texture handle directly with webrender.

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

::: gfx/webrender_bindings/WebRenderTypes.h
@@ +43,5 @@
>  
>  inline Maybe<WrImageFormat>
>  SurfaceFormatToWrImageFormat(gfx::SurfaceFormat aFormat) {
>    switch (aFormat) {
> +    case gfx::SurfaceFormat::R8G8B8X8:

Yes.
wr::ImageDescriptor descriptor(wrTexture->GetSize(), wrTexture->GetReadFormat());

We will get R8G8B8X8 here.
https://hg.mozilla.org/mozilla-central/annotate/3364cc17988c013c36f2a8123315db2855393011/gfx/layers/composite/TextureHost.h#l416
Comment on attachment 8851335 [details] [diff] [review]
create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

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

::: gfx/layers/composite/TextureHost.cpp
@@ +194,5 @@
>  TextureHost::Create(const SurfaceDescriptor& aDesc,
>                      ISurfaceAllocator* aDeallocator,
>                      LayersBackend aBackend,
>                      TextureFlags aFlags)
>  {

In WebRenderTextureHost::CreateRenderTextureHost(), the default behavior is a gfx critical error. If we turn on webrender, We should only receive BufferTextureHost and MacIOSurfaceTextureHost from client. It's much easier to catch the non-implemented texture type with this approach.

And I will handle the directxYCbCr texture host at another bug.

void
WebRenderTextureHost::CreateRenderTextureHost(const layers::SurfaceDescriptor& aDesc,
                                              TextureHost* aTexture)
	
{
  ...
  switch (aDesc.type()) {
    case SurfaceDescriptor::TSurfaceDescriptorBuffer: {
      ...
      break;
    }
#ifdef XP_MACOSX
    case SurfaceDescriptor::TSurfaceDescriptorMacIOSurface: {
      ...
      break;
    }
	
#endif
    default:
      gfxCriticalError() << "No WR implement for texture type:" << aDesc.type();
	
  }	
}
Created attachment 8853292 [details] [diff] [review]
P1: add WrExternalImage binding utility function. r=nical,sotaro

rebase.
Created attachment 8853293 [details] [diff] [review]
P2: use WrImageDescriptor for wr_api_add_external_image_handle(). r=nical,sotaro

rebase
Created attachment 8853294 [details] [diff] [review]
P3: rename from RenderTextureHost to RenderBufferTextureHost for further updating.

Update for comment 13.
We will have a class RenderTextureHost as the base-class and its sub-class RenderBufferTextureHost in next patch. Use "rename" to preserve the file history.
Attachment #8853294 - Flags: review?(sotaro.ikeda.g)
Created attachment 8853295 [details] [diff] [review]
P4: create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL.

Update for comment 11.
Attachment #8853295 - Flags: review?(sotaro.ikeda.g)
Created attachment 8853296 [details] [diff] [review]
P5: use texture handle directly with webrender. r=nical,sotaro

rebase
(Assignee)

Updated

a year ago
Attachment #8851333 - Attachment is obsolete: true
Attachment #8851334 - Attachment is obsolete: true
Attachment #8851335 - Attachment is obsolete: true
Attachment #8851336 - Attachment is obsolete: true
Attachment #8851335 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Updated

a year ago
Flags: needinfo?(nical.bugzilla)
(Assignee)

Updated

a year ago
Attachment #8853292 - Attachment description: P1: add WrExternalImage binding utility function. → P1: add WrExternalImage binding utility function. r=nical,sotaro
(Assignee)

Updated

a year ago
Attachment #8853293 - Attachment description: P2: use WrImageDescriptor for wr_api_add_external_image_handle(). → P2: use WrImageDescriptor for wr_api_add_external_image_handle(). r=nical,sotaro
(Assignee)

Updated

a year ago
Attachment #8853296 - Attachment description: P5: use texture handle directly with webrender. → P5: use texture handle directly with webrender. r=nical,sotaro
Attachment #8853294 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8853295 - Flags: review?(sotaro.ikeda.g) → review+
Created attachment 8853350 [details] [diff] [review]
P4: create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL. v2. r=nical,sotaro

Fix build break on try server.
(Assignee)

Updated

a year ago
Attachment #8853295 - Attachment is obsolete: true

Comment 23

a year ago
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/f9813e4134ae
P1: add WrExternalImage binding utility function. r=nical,sotaro
https://hg.mozilla.org/projects/graphics/rev/4b54605f1dcd
P2: use WrImageDescriptor for wr_api_add_external_image_handle(). r=nical,sotaro
https://hg.mozilla.org/projects/graphics/rev/5aea276a5283
P3: rename from RenderTextureHost to RenderBufferTextureHost for further updating. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/66e555be3167
P4: create RenderBufferTextureHost and RenderMacIOSurfaceTextureHostOGL. r=nical,sotaro
https://hg.mozilla.org/projects/graphics/rev/34976aba2753
P5: use texture handle directly with webrender. r=nical,sotaro
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Updated

9 months ago
Depends on: 1380578
You need to log in before you can comment on or make changes to this bug.