Closed Bug 1362049 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 14 obsolete files)

4.72 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.14 KB, patch
nical
: review+
Details | Diff | Splinter Review
13.83 KB, patch
Details | Diff | Splinter Review
2.89 KB, patch
nical
: review+
Details | Diff | Splinter Review
14.13 KB, patch
nical
: review+
Details | Diff | Splinter Review
1.67 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
13.72 KB, patch
Details | Diff | Splinter Review
4.77 KB, patch
Details | Diff | Splinter Review
5.74 KB, patch
Details | Diff | Splinter Review
This bug try to handle the NV12 and YUV422 format MacIOSurface for video playback.
The MacIOSurfaceTextureSourceOGL doesn't be used in the codebase.

MozReview-Commit-ID: EZ1fHw7J6YD
Attachment #8864544 - Flags: review?(matt.woodrow)
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
Attachment #8864545 - Flags: review?(matt.woodrow)
Attached patch Add RG8 type in SurfaceFormat. (obsolete) — Splinter Review
This RG8 format will be used in NV12 image format.

MozReview-Commit-ID: KNJFwOasVts
Attachment #8864546 - Flags: review?(nical.bugzilla)
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
Attachment #8864547 - Flags: review?(nical.bugzilla)
Different TextureHost type could use different WR commands. So, make a abstract interface for these different commands.

MozReview-Commit-ID: 63dnOJC2P9r
Attachment #8864548 - Flags: review?(nical.bugzilla)
MozReview-Commit-ID: JJlqFwidliQ
Attachment #8864550 - Flags: review?(nical.bugzilla)
MozReview-Commit-ID: 3LaWdfglobe
Attachment #8864552 - Flags: review?(nical.bugzilla)
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
Attachment #8864553 - Flags: review?(nical.bugzilla)
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).
Attachment #8864553 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
Attachment #8864547 - Flags: review?(nical.bugzilla) → review+
Attachment #8864544 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8864553 [details] [diff] [review]
Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL.

Matt's an even better reviewer than me.
Attachment #8864553 - Flags: review?(jmuizelaar) → review?(matt.woodrow)
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.
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.
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.
Attachment #8864768 - Flags: review?(matt.woodrow)
Attachment #8864553 - Attachment is obsolete: true
Attachment #8864553 - Flags: review?(matt.woodrow)
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
Attachment #8864769 - Flags: review?(matt.woodrow)
Attachment #8864545 - Attachment is obsolete: true
Attachment #8864545 - Flags: review?(matt.woodrow)
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.
Attachment #8864769 - Flags: review?(matt.woodrow) → review+
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?
Attachment #8864768 - Flags: review?(matt.woodrow) → review+
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 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.
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
Attachment #8864769 - Attachment is obsolete: true
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
Attachment #8864768 - Attachment is obsolete: true
Attachment #8865280 - Attachment description: Update the texture target and texture format in MacIOSurface. v3. → Update the texture target and texture format in MacIOSurface. v3. r=mattwoodrow
Attachment #8865281 - Attachment description: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v3. → Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v3. r=mattwoodrow
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.
Attachment #8864548 - Flags: review?(nical.bugzilla) → review-
(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.
Attachment #8864550 - Flags: review?(nical.bugzilla) → review+
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).
Attachment #8864552 - Flags: review?(nical.bugzilla) → review+
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?
(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.
Flags: needinfo?(bas)
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.
Attachment #8864546 - Attachment is obsolete: true
Attachment #8864546 - Flags: review?(nical.bugzilla)
rebase.

MozReview-Commit-ID: 1kDerOGwUuE
Attachment #8864547 - Attachment is obsolete: true
Different TextureHost type could use different WR commands. So, make a abstract interface for these different commands.

MozReview-Commit-ID: 63dnOJC2P9r
Attachment #8867687 - Flags: review?(nical.bugzilla)
Attachment #8864548 - Attachment is obsolete: true
Attachment #8864550 - Attachment is obsolete: true
Use the MacIOSurface with APPLE_rgb_422 format directly in WR.

MozReview-Commit-ID: 19GLtuJLiPv
Attachment #8867689 - Flags: review?(nical.bugzilla)
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.
Flags: needinfo?(bas)
Attachment #8864544 - Attachment description: Remove the unused MacIOSurfaceTextureSourceOGL. → P1: Remove the unused MacIOSurfaceTextureSourceOGL.
Attachment #8865280 - Attachment description: Update the texture target and texture format in MacIOSurface. v3. r=mattwoodrow → P2: Update the texture target and texture format in MacIOSurface. v3. r=mattwoodrow
Attachment #8867686 - Attachment description: Update the ext-image interface in WR binding. r=nical. v2 → P3: Update the ext-image interface in WR binding. r=nical. v2
Attachment #8867687 - Attachment description: Add AddWRImage() to call the proper WR commands for all TextureHost types. v2 → P4: Add AddWRImage() to call the proper WR commands for all TextureHost types. v2
Attachment #8867688 - Attachment description: The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations. r=nical. v2 → P5: The BufferTextureHost::AddWRImage() and MacIOSurfaceTextureHostOGL::AddWRImage() implementations. r=nical. v2
Attachment #8864552 - Attachment description: Use channel_index to get the correct channel data info from RenderTextureHost. → P6: Use channel_index to get the correct channel data info from RenderTextureHost.
Attachment #8865281 - Attachment description: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v3. r=mattwoodrow → P7: Handle multiple-channel format for RenderMacIOSurfaceTextureHostOGL. v3. r=mattwoodrow
Attachment #8867689 - Attachment description: Use the MacIOSurface texture directly for video playback. → P8: Use the MacIOSurface texture directly for video playback.
Attachment #8867687 - Attachment is obsolete: true
Attachment #8867687 - Flags: review?(nical.bugzilla)
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
Attachment #8868085 - Flags: review?(bas)
Attachment #8867689 - Flags: review?(nical.bugzilla) → review+
Attachment #8868062 - Flags: review?(nical.bugzilla) → review+
This should then be R8G8, not RG8, or RG16, depending on endianness.
Update GL_R to GL_RED.

MozReview-Commit-ID: 3zPfiAmaFBz
Attachment #8865280 - Attachment is obsolete: true
This R8G8 format will be used in NV12 image format.

MozReview-Commit-ID: KNJFwOasVts
Attachment #8868477 - Flags: review?(bas)
Attachment #8868085 - Attachment is obsolete: true
Attachment #8868085 - Flags: review?(bas)
Attachment #8867688 - Attachment is obsolete: true
Attachment #8868449 - Attachment is obsolete: true
Attachment #8865281 - Attachment is obsolete: true
Attachment #8868477 - Flags: review?(bas) → review+
Pushed by hshih@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.