1362049 - [WR] handle the multiple-channel MacIOSurface for video playback

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

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

This bug try to handle the NV12 and YUV422 format MacIOSurface for video playback.

Comment 1

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

Attachment: P1: Remove the unused MacIOSurfaceTextureSourceOGL.

The MacIOSurfaceTextureSourceOGL doesn't be used in the codebase.

MozReview-Commit-ID: EZ1fHw7J6YD

Comment 2

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

Attachment: Update the texture target and texture format in MacIOSurface.

Add LOCAL_GL prefix for all GL constant value.
Turn to use GL_R and GL_RB if we use core profile.
Turn to use GL_RGB_422_APPLE instead of GL_YCBCR_422_APPLE for core profile.

MozReview-Commit-ID: 3zPfiAmaFBz

Comment 3

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

Attachment: Add RG8 type in SurfaceFormat.

This RG8 format will be used in NV12 image format.

MozReview-Commit-ID: KNJFwOasVts

Comment 4

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

Attachment: Update the ext-image interface in WR binding.

Use wr_api_add_external_image() to replace all types of ext-image adding functions.
Add a new interface wr_dp_push_yuv_interleaved_image() to put a single channel yuv_interleaved image in WR display list.

MozReview-Commit-ID: 1kDerOGwUuE

Comment 5

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

Attachment: Add AddWRImage() to call the proper WR commands for all TextureHost types.

Different TextureHost type could use different WR commands. So, make a abstract interface for these different commands.

MozReview-Commit-ID: 63dnOJC2P9r

Comment 6

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

Attachment: The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations.

MozReview-Commit-ID: JJlqFwidliQ

Comment 7

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

Attachment: P6: Use channel_index to get the correct channel data info from RenderTextureHost.

MozReview-Commit-ID: 3LaWdfglobe

Comment 8

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

Attachment: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL.

We could use NV12 or YCbCr-planar format for video playback. There will be up to 3 channels in the MacIOSurface.

MozReview-Commit-ID: 77RYntphjYy

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8864553 [details] [diff] [review]
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL.

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

I don't know much about MacIoSurface, Jeff will be a better reviewer for this.

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.cpp
@@ +110,5 @@
> +    // be 2 if the format has 2 planar data.
> +    mGL->fDeleteTextures(1, &(mTextureHandle[0]));
> +    mTextureHandle[0] = 0;
> +    for (size_t i = 1; i < mSurface->GetPlaneCount(); ++i) {
> +      mGL->fDeleteTextures(1, &(mTextureHandle[i]));

nit: you can just mGl->fDeleteTextures(planeCount, mTextureHandle);
with plane count being 1 intead of 0 when GetPlaneCount returns 0 (which is a bit confusing btw)

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.h
@@ +41,5 @@
>    void DeleteTextureHandle();
>  
>    RefPtr<MacIOSurface> mSurface;
>    RefPtr<gl::GLContext> mGL;
> +  GLuint mTextureHandle[3];

nit: mTextureHandles (plural).

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8864553 [details] [diff] [review]
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL.

Matt's an even better reviewer than me.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8864545 [details] [diff] [review]
Update the texture target and texture format in MacIOSurface.

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

::: gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp
@@ +35,5 @@
>    gl->fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);
>    gl->fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
>  
> +  mSurface->CGLTexImageIOSurface2D(gl::GLContextCGL::Cast(gl)->GetCGLContext(),
> +                                   gl->IsCompatibilityProfile(),

If this returns false, then won't we read the wrong format, since our shaders are setup expecting RGB?

I guess this is an existing problem.

Can we just assert that IsCompatibilityProfile() == true, and then pass true to this function?

It might be worth having this function return the 'read' format (as retval, or outparam), since it's a bit weird that it silently decides between YUV/RGB, yet you need to pick the appropriate shader.

If you do that, then we can just assert that the read type is rgb.

Comment 12

[Matt Woodrow (:mattwoodrow)]

I'm also somewhat surprised that GL_YCBCR_422_APPLE works at all, given that the docs for IOSurface say:

"Because of the way IOSurface is currently implemented, it does not allow for any kind of
 automatic format conversion to take place between the data in the IOSurface and the data seen
 by the GPU.   This means that OpenGL is going to interpret the data in the IOSurface exactly
 as how it is specified by the format and type parameters passed to CGLTexImageIOSurface2D()"

I guess it does though, since it appears to be being used.

Comment 13

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

Attachment: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v2

Make sure there is the YUV422 MacIOSurface for webrender.
The webrender only supports YUV422 read format for YUV422 image now.
By the way, CompositorOGL only supports rgb read format for YUV422 image.

Comment 14

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

Attachment: Update the texture target and texture format in MacIOSurface. v2

Add LOCAL_GL prefix for all GL constant value.
Turn to use GL_R and GL_RB if we use core profile.
Turn to use GL_RGB_422_APPLE instead of GL_YCBCR_422_APPLE for core profile.
Make sure we only have rgb read format for YUV422 image for CompositorOGL.

MozReview-Commit-ID: 3zPfiAmaFBz

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8864769 [details] [diff] [review]
Update the texture target and texture format in MacIOSurface. v2

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

::: gfx/2d/MacIOSurface.cpp
@@ +557,5 @@
> +      if (aOutReadFormat) {
> +        *aOutReadFormat = mozilla::gfx::SurfaceFormat::R8G8B8X8;
> +      }
> +    } else {
> +      format = LOCAL_GL_RGB_422_APPLE;

Should the internal format be RGB_RAW_422_APPLE here?

::: gfx/gl/GLBlitHelper.cpp
@@ +792,5 @@
>          mGL->fActiveTexture(LOCAL_GL_TEXTURE0 + i);
>          mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldTex[i]);
>      }
>  
> +    bool isCompatibilityProfile = mGL->IsCompatibilityProfile();

This is unused.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8864768 [details] [diff] [review]
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v2

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

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.cpp
@@ +39,4 @@
>  {
>    MOZ_COUNT_CTOR_INHERITED(RenderMacIOSurfaceTextureHostOGL, RenderTextureHostOGL);
>  
> +  mTextureHandle[0] = 0;

I think you should be to initialize this in the initializer list as : mTextureHandle({0, 0, 0})

@@ +116,5 @@
>  {
> +  if (mTextureHandle[0] != 0 && mGL && mGL->MakeCurrent()) {
> +    // The result of GetPlaneCount() is 0 for a single planar format, but it will
> +    // be 2 if the format has 2 planar data.
> +    mGL->fDeleteTextures(1, &(mTextureHandle[0]));

Calling glDeleteTextures on 0 isn't an error, so you could just call fDeleteTextures(3, ..) here always, instead of looping.

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.h
@@ +41,5 @@
>    void DeleteTextureHandle();
>  
>    RefPtr<MacIOSurface> mSurface;
>    RefPtr<gl::GLContext> mGL;
> +  GLuint mTextureHandle[3];

mTextureHandles?

Comment 17

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

Comment on attachment 8864768 [details] [diff] [review]
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v2

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

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.cpp
@@ +116,5 @@
>  {
> +  if (mTextureHandle[0] != 0 && mGL && mGL->MakeCurrent()) {
> +    // The result of GetPlaneCount() is 0 for a single planar format, but it will
> +    // be 2 if the format has 2 planar data.
> +    mGL->fDeleteTextures(1, &(mTextureHandle[0]));

check.

::: gfx/webrender_bindings/RenderMacIOSurfaceTextureHostOGL.h
@@ +41,5 @@
>    void DeleteTextureHandle();
>  
>    RefPtr<MacIOSurface> mSurface;
>    RefPtr<gl::GLContext> mGL;
> +  GLuint mTextureHandle[3];

It's updated now.

Comment 18

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

Comment on attachment 8864769 [details] [diff] [review]
Update the texture target and texture format in MacIOSurface. v2

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

::: gfx/2d/MacIOSurface.cpp
@@ +557,5 @@
> +      if (aOutReadFormat) {
> +        *aOutReadFormat = mozilla::gfx::SurfaceFormat::R8G8B8X8;
> +      }
> +    } else {
> +      format = LOCAL_GL_RGB_422_APPLE;

No, the internal format is still the rgb.

Here is a example:
http://git.videolan.org/?p=vlc.git;a=blobdiff;f=modules/video_output/opengl/converter_cvpx.c;h=31ed3fee8a8c60ea314a23c662bc7963a3ec8381;hp=75f21244f4330912383801602df00069ed43c143;hb=d284a2edefe14278568343cb639b606d6afc7fee;hpb=5fdb33e474ee6332f37030cc548de4d2a8094c1d

::: gfx/gl/GLBlitHelper.cpp
@@ +792,5 @@
>          mGL->fActiveTexture(LOCAL_GL_TEXTURE0 + i);
>          mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldTex[i]);
>      }
>  
> +    bool isCompatibilityProfile = mGL->IsCompatibilityProfile();

It's removed now.

Comment 19

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

Attachment: P2: Update the texture target and texture format in MacIOSurface. v3. r=mattwoodrow

Add LOCAL_GL prefix for all GL constant value.
Turn to use GL_R and GL_RB if we use core profile.
Turn to use GL_RGB_422_APPLE instead of GL_YCBCR_422_APPLE for core profile.

MozReview-Commit-ID: 3zPfiAmaFBz

Comment 20

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

Attachment: P7: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v3. r=mattwoodrow

We could use NV12 or YCbCr-planar format for video playback. There will be up to 3 channels in the MacIOSurface.

MozReview-Commit-ID: 77RYntphjYy

Comment 21

[Nicolas Silva [:nical]]

Comment on attachment 8864548 [details] [diff] [review]
Add AddWRImage() to call the proper WR commands for all TextureHost types.

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

Please document what it means for this method to return true or false. It looks like the return value is not used either and I suppose it should. A lot of the implementations are empty, please either add a comment explaining why there's nothing to do, or an assertion if we are not expecting it to be called.

Comment 22

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #21)
> A lot of the implementations are empty, please either add a comment explaining
> why there's nothing to do, or an assertion if we are not expecting it to be
> called.

I see that the next patch addresses part of this.

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8864552 [details] [diff] [review]
P6: Use channel_index to get the correct channel data info from RenderTextureHost.

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

r=me if you replace RenderBufferData, see below.

::: gfx/webrender_bindings/RenderBufferTextureHost.h
@@ +24,5 @@
>    {
>      return this;
>    }
>  
> +  class RenderBufferData

Please use mfbt Range<uint8_t> for this.

See https://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/mfbt/Range.h#20

::: gfx/webrender_bindings/RendererOGL.cpp
@@ +30,2 @@
>  
> +    return RawDataToWrExternalImage(data.mData, data.mBufferSize);

Note: it would be better if this took an mfbt Range<uint8_t> as parameter instead of the raw data (you don't have to fix it in this path).

Comment 24

[Nicolas Silva [:nical]]

Comment on attachment 8864546 [details] [diff] [review]
Add RG8 type in SurfaceFormat.

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

What is the layout of a texture with this format, is it 24 bit RGB?

Comment 25

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #24)
> Comment on attachment 8864546 [details] [diff] [review]
> Add RG8 type in SurfaceFormat.
> 
> Review of attachment 8864546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the layout of a texture with this format, is it 24 bit RGB?

Sorry I read RGB8 instead of RG8. Sounds good to me if Bas is ok with adding an RG8 format to moz2d.

Comment 26

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

Comment on attachment 8864546 [details] [diff] [review]
Add RG8 type in SurfaceFormat.

This format is used for the CbCr channel in NV12 format.
We don't use that for current mac hw-video decoding setting. So, make it obsolete here.

Comment 27

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

Attachment: P3: Update the ext-image interface in WR binding. r=nical. v2

rebase.

MozReview-Commit-ID: 1kDerOGwUuE

Comment 28

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

Attachment: P4: Add AddWRImage() to call the proper WR commands for all TextureHost types. v2

Different TextureHost type could use different WR commands. So, make a abstract interface for these different commands.

MozReview-Commit-ID: 63dnOJC2P9r

Comment 29

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

Attachment: P5: The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations. r=nical. v2

rebase.

MozReview-Commit-ID: JJlqFwidliQ

Comment 30

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

Attachment: P8: Use the MacIOSurface texture directly for video playback.

Use the MacIOSurface with APPLE_rgb_422 format directly in WR.

MozReview-Commit-ID: 19GLtuJLiPv

Comment 31

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

Comment on attachment 8867687 [details] [diff] [review]
P4: Add AddWRImage() to call the proper WR commands for all TextureHost types. v2

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

Remove the return value.
void AddWRImage(wr::WebRenderAPI* aAPI, const wr::ImageKey& aImageKey, const wr::ExternalImageId& aExtID)

Add MOZ_ASSERT_UNREACHABLE() for the non-implemented function.

Comment 32

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

Attachment: P4: Add AddWRImage() to call the proper WR commands for all TextureHost types. v3.

rebase

Comment 33

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

Attachment: P9: Add RG8 type in SurfaceFormat.

This RG8 format will be used in "NV12" image format.

In NV12, we will have a channel for CbCr data. And we will use RG8 in WR for that.

MozReview-Commit-ID: KNJFwOasVts

Comment 34

[Bas Schouten (:bas.schouten)]

This should then be R8G8, not RG8, or RG16, depending on endianness.

Comment 35

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

Attachment: P2: Update the texture target and texture format in MacIOSurface. v4. r=mattwoodrow

Update GL_R to GL_RED.

MozReview-Commit-ID: 3zPfiAmaFBz

Comment 36

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

Attachment: P0: Add R8G8 type in SurfaceFormat. v2.

This R8G8 format will be used in NV12 image format.

MozReview-Commit-ID: KNJFwOasVts

Comment 37

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

Attachment: P2: Update the texture target and texture format in MacIOSurface. v5.

fix build break.

Comment 38

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

Attachment: P5: The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations. v3. r=nical

Update RG8 to R8G8.

Comment 39

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

There is a resource_cache bug in WR.
Wait for https://github.com/servo/webrender/pull/1261 .

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c919bdca0277be5f3a36ea74c4d30d8300d59fd8

Comment 40

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

Attachment: P7: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v4. r=mattwoodrow

fix build break.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e55e07e229ba
Add R8G8 type in SurfaceFormat. r=bas.schouten
https://hg.mozilla.org/projects/graphics/rev/9f53823528be
Remove the unused MacIOSurfaceTextureSourceOGL. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/d87f22c0cfbe
Update the texture target and texture format in MacIOSurface. v5. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/a26381a60aa6
Update the ext-image interface in WR binding. v2. r=nical
https://hg.mozilla.org/projects/graphics/rev/5828e739a454
Add AddWRImage() to call the proper WR commands for all TextureHost types. v3. r=nical
https://hg.mozilla.org/projects/graphics/rev/f7cb82a21940
The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations. v3. r=nical
https://hg.mozilla.org/projects/graphics/rev/c74b9d026772
Use channel_index to get the correct channel data info from RenderTextureHost. r=nical
https://hg.mozilla.org/projects/graphics/rev/74c0e29fa0fb
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v4. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/ee2ef506dfb9
Use the MacIOSurface texture directly for video playback. r=nical

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/e55e07e229ba
https://hg.mozilla.org/mozilla-central/rev/9f53823528be
https://hg.mozilla.org/mozilla-central/rev/d87f22c0cfbe
https://hg.mozilla.org/mozilla-central/rev/a26381a60aa6
https://hg.mozilla.org/mozilla-central/rev/5828e739a454
https://hg.mozilla.org/mozilla-central/rev/f7cb82a21940
https://hg.mozilla.org/mozilla-central/rev/c74b9d026772
https://hg.mozilla.org/mozilla-central/rev/74c0e29fa0fb
https://hg.mozilla.org/mozilla-central/rev/ee2ef506dfb9