Add utility to create CVPixelBuffers from IOSurfaces

[Kimmo Kinnunen]

Add utility to create CVPixelBuffers from IOSurfaces

[Kimmo Kinnunen]

Created attachment 438338 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438339 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438340 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438378 [details]
Patch

[Eric Carlson]

Comment on attachment 438378 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:84
> +#if PLATFORM(MAC)
> +    auto format = IOSurfaceGetPixelFormat(surface);
> +    if (format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange || format == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) {
> +        constexpr size_t macroblockSize = 16;
> +        auto width = IOSurfaceGetWidth(surface);
> +        auto height = IOSurfaceGetHeight(surface);
> +        auto extendedRight = roundUpToMultipleOf(macroblockSize, width) - width;
> +        auto extendedBottom = roundUpToMultipleOf(macroblockSize, height) - height;
> +        if ((IOSurfaceGetBytesPerRowOfPlane(surface, 0) >= width + extendedRight)
> +            && (IOSurfaceGetBytesPerRowOfPlane(surface, 1) >= width + extendedRight)
> +            && (IOSurfaceGetAllocSize(surface) >= (height + extendedBottom) * IOSurfaceGetBytesPerRowOfPlane(surface, 0) * 3 / 2)) {
> +            return (__bridge CFDictionaryRef) @{
> +#if PLATFORM(MAC)
> +                (__bridge NSString *)kCVPixelBufferOpenGLCompatibilityKey : @YES,
> +#endif

Why have nested `#if PLATFORM(MAC)` ?

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:102
> +    if (!surface)

Might be worth ASSERT-ing this

[Kimmo Kinnunen]

Created attachment 438382 [details]
Patch

[Darin Adler]

Comment on attachment 438382 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:72
> +    if (format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange || format == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) {

I think this needs a brief "why" comment.

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:100
> +    ASSERT(surface);

Why assert this? We’re not asserting pixelBufferPool above, for example.

Separately, but related: Maybe we could do a null check, return an unexpected value in that case, and remove the null check in ImageTransferSessionVT::createCMSampleBuffer?

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:102
> +        if (auto pool = createIOSurfaceCVPixelBufferPool({ static_cast<int>(width), static_cast<int>(height) }, poolBufferType)) {

Not new, but what guarantees the width and height fit into int?

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:500
> +        m_pixelBufferPool = WebCore::createIOSurfaceCVPixelBufferPool({ static_cast<int>(width), static_cast<int>(height) }, type).value_or(nullptr);

Not new, but what guarantees the width and height fit into int?

[Darin Adler]

Comment on attachment 438382 [details]
Patch

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

>> Source/WebCore/platform/graphics/cv/CVUtilities.mm:100
>> +    ASSERT(surface);
> 
> Why assert this? We’re not asserting pixelBufferPool above, for example.
> 
> Separately, but related: Maybe we could do a null check, return an unexpected value in that case, and remove the null check in ImageTransferSessionVT::createCMSampleBuffer?

I see now that my comment is the opposite of Eric’s!

[Eric Carlson]

Comment on attachment 438382 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cv/CVUtilities.mm:100
>>> +    ASSERT(surface);
>> 
>> Why assert this? We’re not asserting pixelBufferPool above, for example.
>> 
>> Separately, but related: Maybe we could do a null check, return an unexpected value in that case, and remove the null check in ImageTransferSessionVT::createCMSampleBuffer?
> 
> I see now that my comment is the opposite of Eric’s!

I suggested the change because CVPixelBufferCreateWithIOSurface already returns an error when passed a NULL surface, but I think it would be useful to catch this in debug builds.

[Kimmo Kinnunen]

Created attachment 438448 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438500 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438506 [details]
Patch

[Kimmo Kinnunen]

> > Source/WebCore/platform/graphics/cv/CVUtilities.mm:72
> > +    if (format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange || format == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) {
> 
> I think this needs a brief "why" comment.

Eric, do you have a wording for why?
I know roughly what it does, but not exactly why.

git log --oneline -S "roundUpToMacroblockMultiple" -- Source/WebCore/platform 

53093dc43c75 [MediaStream] Consolidate all image conversion and resizing into one class https://bugs.webkit.org/show_bug.cgi?id=190519 <rdar://problem/45224307>
600ac42b961f [MediaStream] Restructure getDisplayMedia classes https://bugs.webkit.org/show_bug.cgi?id=187905
342759df7ad6 [MediaStream] Add Mac screen capture source https://bugs.webkit.org/show_bug.cgi?id=181333 <rdar://problem/36323219>

I tried to formulate something, unsure if it's any better or exactly correct.


> Not new, but what guarantees the width and height fit into int?
Reverted these to the original.

>>>> +    ASSERT(surface);
>>> Why assert this? We’re not asserting pixelBufferPool above, for example.

Reverted the assert.

[youenn fablet]

Comment on attachment 438506 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:55
> +        return makeUnexpected(status);

We do not seem to log errors at call sites of createIOSurfaceCVPixelBufferPool.
Maybe we should, or should do here.

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:64
> +        return makeUnexpected(status);

It is a bit odd to have a case where kCVReturnSuccess and pixelBuffer is null.
But it seems good to make it the error path.

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:79
> +    auto bufferPool = createIOSurfaceCVPixelBufferPool(size.width(), size.height(), m_pixelFormat, 6).value_or(nullptr);

Preexisting issue but this value 6 is a bit mysterious.

> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:214
> +    auto pixelBuffer = WebCore::createCVPixelBuffer(sample.surface()).value_or(nullptr);

If pixelBuffer is null, we probably want to exit early, maybe log an error.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:500
> +        m_pixelBufferPool = WebCore::createIOSurfaceCVPixelBufferPool(width, height, type).value_or(nullptr);

We could not set m_pixelBufferPoolWidth/m_pixelBufferPoolHeight if m_pixelBufferPool creation failed.

[Radar WebKit Bug Importer]

<rdar://problem/83440494>

[Kimmo Kinnunen]

Created attachment 439133 [details]
Patch

[EWS]

Committed r283036 (242096@main): <https://commits.webkit.org/242096@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439133 [details].