Bug 230338

Created attachment 438338 [details] Patch

Created attachment 438339 [details] Patch

Created attachment 438340 [details] Patch

Created attachment 438378 [details] Patch

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

Created attachment 438382 [details] Patch

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?

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!

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.

Created attachment 438448 [details] Patch

Created attachment 438500 [details] Patch

Created attachment 438506 [details] Patch

> > 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.

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.

<rdar://problem/83440494>

Created attachment 439133 [details] Patch

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].