Performance: do pixel conformance and texturing in a single step.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

Performance: do pixel conformance and texturing in a single step.

[Jer Noble]

<rdar://problem/34937237>

[Jer Noble]

Created attachment 323536 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 323536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323536&action=review

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:84
> +    if (string == kCVImageBufferYCbCrMatrix_ITU_R_709_2)

Does this need to do actual string comparison?

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:126
> +        matrices.get().emplace(MapKey(PixelRange::Video, TransferFunction::kITU_R_601_4), Vector<GLfloat>({
> +            1.164383562f,  0.0f,           1.596026786f,
> +            1.164383562f, -0.3917622901f, -0.8129676472f,
> +            1.164383562f,  2.017232143f,   0.0f,
> +        }));
> +        matrices.get().emplace(MapKey({PixelRange::Video, TransferFunction::kITU_R_709_2}), Vector<GLfloat>({
> +            1.164383562f,  0.0f,           1.792741071f,
> +            1.164383562f, -0.2132486143f, -0.5329093286f,
> +            1.164383562f,  2.112401786f,   0.0f,
> +        }));
> +        matrices.get().emplace(MapKey({PixelRange::Full, TransferFunction::kITU_R_601_4}), Vector<GLfloat>({
> +            1.000000000f,  0.0f,           1.4075196850f,
> +            1.000000000f, -0.3454911535f, -0.7169478464f,
> +            1.000000000f,  1.7789763780f,  0.0f,
> +        }));
> +        matrices.get().emplace(MapKey({PixelRange::Full, TransferFunction::kITU_R_709_2}), Vector<GLfloat>({
> +            1.000000000f,  0.0f,           1.5810000000f,
> +            1.000000000f, -0.1880617701f, -0.4699672819f,
> +            1.000000000f,  1.8629055118f,  0.0f,
> +        }));

Would be nice to document where the magic numbers come from.

[Jer Noble]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 323536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323536&action=review
> 
> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:84
> > +    if (string == kCVImageBufferYCbCrMatrix_ITU_R_709_2)
> 
> Does this need to do actual string comparison?

Nope. They use the exact pointers of the exported constants.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:126
> > +        matrices.get().emplace(MapKey(PixelRange::Video, TransferFunction::kITU_R_601_4), Vector<GLfloat>({
> > +            1.164383562f,  0.0f,           1.596026786f,
> > +            1.164383562f, -0.3917622901f, -0.8129676472f,
> > +            1.164383562f,  2.017232143f,   0.0f,
> > +        }));
> > +        matrices.get().emplace(MapKey({PixelRange::Video, TransferFunction::kITU_R_709_2}), Vector<GLfloat>({
> > +            1.164383562f,  0.0f,           1.792741071f,
> > +            1.164383562f, -0.2132486143f, -0.5329093286f,
> > +            1.164383562f,  2.112401786f,   0.0f,
> > +        }));
> > +        matrices.get().emplace(MapKey({PixelRange::Full, TransferFunction::kITU_R_601_4}), Vector<GLfloat>({
> > +            1.000000000f,  0.0f,           1.4075196850f,
> > +            1.000000000f, -0.3454911535f, -0.7169478464f,
> > +            1.000000000f,  1.7789763780f,  0.0f,
> > +        }));
> > +        matrices.get().emplace(MapKey({PixelRange::Full, TransferFunction::kITU_R_709_2}), Vector<GLfloat>({
> > +            1.000000000f,  0.0f,           1.5810000000f,
> > +            1.000000000f, -0.1880617701f, -0.4699672819f,
> > +            1.000000000f,  1.8629055118f,  0.0f,
> > +        }));
> 
> Would be nice to document where the magic numbers come from.

Sure, I can add links to the ITU specs.

[Jer Noble]

Created attachment 323544 [details]
Patch

[Dean Jackson]

Comment on attachment 323536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323536&action=review

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:131
> +    // Assume unknown pixel ranges are full:
> +    if (range == PixelRange::Unknown)
> +        range = PixelRange::Full;

This comment isn't helpful, but I would like to know when we get something like this and what it means.

Maybe we could ASSERT on an OSType pixelFormat that we don't handle?

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:393
> +    GC3Dint value = 0;

Maybe call this compileStatus? Or status since you use it for link status too.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:396
> +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Vertex shader failed to compile.", this);

This function is now named initializeUVContextObjects

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:429
> +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Fragment shader failed to compile.", this);

Ditto.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:442
> +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Program failed to link.", this);

Ditto.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:486
> +    if (pixelFormat != kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange && pixelFormat != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange)
> +        return false;

I think we should log here.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:523
> +    // Bind and set up the texture for the video source.

Nit: textures plural

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:530
> +    GC3Denum videoTextureTarget = GL_TEXTURE_RECTANGLE_ARB;

I don't know if we need this local variable.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:537
> +    if (kCGLNoError != CGLTexImageIOSurface2D(m_context->platformGraphicsContext3D(), videoTextureTarget, GL_RG, uvPlaneWidth, uvPlaneHeight, GL_RG, GL_UNSIGNED_BYTE, surface, 1)) {

Why do you do plane/texture 1 before plane/texture 0?

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:572
> +    m_context->enableVertexAttribArray(m_yuvPositionAttributeLocation);
> +    m_context->bindBuffer(GraphicsContext3D::ARRAY_BUFFER, m_yuvVertexBuffer);
> +    m_context->vertexAttribPointer(m_yuvPositionAttributeLocation, 2, GraphicsContext3D::FLOAT, false, 0, 0);

While it makes no difference, I'd swap the order of lines 2 and 3, so the vertex attrib parts are together.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.h:84
>      bool initializeContextObjects();
> +    bool initializeUVContextObjects();

Do you think we should rename the first one to be clear that it's for the case where we were able to get a GL texture from the source?

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.h:97
> +    Platform3DObject m_yuvVertexBuffer { 0 };

We could potentially use the same vertex buffer for the regular and YUV passes.

[Dean Jackson]

Comment on attachment 323544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323544&action=review

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:-226
> -#if PLATFORM(IOS)
> -            attributes = @{(NSString *)kCVPixelBufferIOSurfaceOpenGLESFBOCompatibilityKey: @YES};
> -#else
> -            attributes = @{(NSString *)kCVPixelBufferIOSurfaceOpenGLFBOCompatibilityKey: @YES};
> -#endif

If we never send these attributes, will we ever get into the copyVideoTextureToPlatformTexture path? Where does the CVOpenGLTextureRef come from (as TextureType)? Will m_textureCache->textureFromImage still work without these options?

[Jer Noble]

(In reply to Dean Jackson from comment #6)
> Comment on attachment 323536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323536&action=review
> 
> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:131
> > +    // Assume unknown pixel ranges are full:
> > +    if (range == PixelRange::Unknown)
> > +        range = PixelRange::Full;
> 
> This comment isn't helpful, but I would like to know when we get something
> like this and what it means.
> 
> Maybe we could ASSERT on an OSType pixelFormat that we don't handle?

Sure, lets do that.  It should only happen when we get some crazy unknown pixel format not listed in CVPixelBuffer.h.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:393
> > +    GC3Dint value = 0;
> 
> Maybe call this compileStatus? Or status since you use it for link status
> too.

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:396
> > +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Vertex shader failed to compile.", this);
> 
> This function is now named initializeUVContextObjects

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:429
> > +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Fragment shader failed to compile.", this);
> 
> Ditto.

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:442
> > +        LOG(WebGL, "VideoTextureCopierCV::copyVideoTextureToPlatformTexture(%p) - Program failed to link.", this);
> 
> Ditto.

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:486
> > +    if (pixelFormat != kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange && pixelFormat != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange)
> > +        return false;
> 
> I think we should log here.

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:523
> > +    // Bind and set up the texture for the video source.
> 
> Nit: textures plural

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:530
> > +    GC3Denum videoTextureTarget = GL_TEXTURE_RECTANGLE_ARB;
> 
> I don't know if we need this local variable.

Ok.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:537
> > +    if (kCGLNoError != CGLTexImageIOSurface2D(m_context->platformGraphicsContext3D(), videoTextureTarget, GL_RG, uvPlaneWidth, uvPlaneHeight, GL_RG, GL_UNSIGNED_BYTE, surface, 1)) {
> 
> Why do you do plane/texture 1 before plane/texture 0?

So that we end with an active 0 unit, same as the other path.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:572
> > +    m_context->enableVertexAttribArray(m_yuvPositionAttributeLocation);
> > +    m_context->bindBuffer(GraphicsContext3D::ARRAY_BUFFER, m_yuvVertexBuffer);
> > +    m_context->vertexAttribPointer(m_yuvPositionAttributeLocation, 2, GraphicsContext3D::FLOAT, false, 0, 0);
> 
> While it makes no difference, I'd swap the order of lines 2 and 3, so the
> vertex attrib parts are together.

Sure.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.h:84
> >      bool initializeContextObjects();
> > +    bool initializeUVContextObjects();
> 
> Do you think we should rename the first one to be clear that it's for the
> case where we were able to get a GL texture from the source?

Maybe? ¯\_(ツ)_/¯ 

I didn't want to touch the other path, if at all possible.

> > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.h:97
> > +    Platform3DObject m_yuvVertexBuffer { 0 };
> 
> We could potentially use the same vertex buffer for the regular and YUV
> passes.

Perhaps, but in my original patch, I used GL_TRIANGLE_STRIP instead of GL_TRIANGLES; now that they both use the same draw call, we could.  But we'd have to add weird detection to both initialization paths to detect whether the value had already been initialized so it didn't get accidentally re-initialized.  I thought it would be simpler to keep them separate.

[Jer Noble]

(In reply to Dean Jackson from comment #7)
> Comment on attachment 323544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323544&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:-226
> > -#if PLATFORM(IOS)
> > -            attributes = @{(NSString *)kCVPixelBufferIOSurfaceOpenGLESFBOCompatibilityKey: @YES};
> > -#else
> > -            attributes = @{(NSString *)kCVPixelBufferIOSurfaceOpenGLFBOCompatibilityKey: @YES};
> > -#endif
> 
> If we never send these attributes, will we ever get into the
> copyVideoTextureToPlatformTexture path?

Yes. If the hardware decoder spits out BRGA on Mac (which it could possibly do, depending on things like the phase of the moon and the weight of your thoughts), that would be GL compatible.  Also, OpenGL ES can texture 420v frames directly, so this path should get hit for all decoded frames.

> Where does the CVOpenGLTextureRef
> come from (as TextureType)? Will m_textureCache->textureFromImage still work
> without these options?

It comes from the texture cache; the texture cache will fail to emit an image if it's not compatible.

[Jer Noble]

Created attachment 323568 [details]
Patch for landing

[Jer Noble]

Created attachment 323584 [details]
Patch for landing

[Jer Noble]

Created attachment 323595 [details]
Patch for landing

[Jer Noble]

Created attachment 323611 [details]
Patch for landing

[Jer Noble]

Created attachment 323647 [details]
Patch for landing

[Jer Noble]

Created attachment 323650 [details]
Patch for landing

[Jer Noble]

Created attachment 323672 [details]
Patch for landing

[Jer Noble]

Created attachment 323675 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 323675 [details]
Patch for landing

Clearing flags on attachment: 323675

Committed r223280: <https://trac.webkit.org/changeset/223280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #18)
> Comment on attachment 323675 [details]
> Patch for landing
> 
> Clearing flags on attachment: 323675
> 
> Committed r223280: <https://trac.webkit.org/changeset/223280>
This change broke the iOS simulator build:
 https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20%28Build%29/builds/683