RemoteSampleBufferDisplayLayer::enqueueSample should not change media samples owned by its object heap
Created attachment 452843 [details] Patch
Comment on attachment 452843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452843&action=review > Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:46 > + , m_sharedVideoFrameReader(nullptr) This should be initialized in the header, not the .cpp file. > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:101 > + IOSurfaceRef surface = pixelBuffer ? CVPixelBufferGetIOSurface(pixelBuffer) : nullptr; > + if (surface) { This looks better to me: if (auto pixelBuffer = downcast<MediaSampleAVFObjC>(frame).pixelBuffer()) { if (auto surface = CVPixelBufferGetIOSurface(pixelBuffer)) { > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:127 > + switchOn(buffer, > + [&](std::nullptr_t representation) { > + encoder << (uint8_t)0; > + }, [&](const RemoteVideoFrameReadReference& reference) { > + encoder << (uint8_t)1; > + encoder << reference; > + } , [&](const MachSendRight& sendRight) { > + encoder << (uint8_t)2; > + encoder << sendRight; > + }); Not sure the indentation here is right. But also, variant should have an encode overload that does this automatically. Why doesn’t this work? encoder << buffer; If it doesn't, we can fix that. > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:161 > + uint8_t bufferType; > + if (!decoder.decode(bufferType)) > + return { }; > + > + if (bufferType > 2) > + return { }; > + > + if (bufferType == 1) { > + std::optional<RemoteVideoFrameReadReference> reference; > + decoder >> reference; > + if (!reference) > + return { }; > + frame.buffer = WTFMove(*reference); > + } else if (bufferType == 2) { > + MachSendRight sendRight; > + if (!decoder.decode(sendRight)) > + return { }; > + frame.buffer = WTFMove(sendRight); > + } > + return frame; This would be *so* much better in the Variant version.
(In reply to Darin Adler from comment #2) > Comment on attachment 452843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452843&action=review > > > Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:46 > > + , m_sharedVideoFrameReader(nullptr) > > This should be initialized in the header, not the .cpp file. This requires including RemoteVideoFrameObjectHeap.h in SharedVideoFrame.h header file. I guess I can change the constructor to take a RemoteVideoFrameObjectHeap* instead of a RefPtr<RemoteVideoFrameObjectHeap>&&. > Not sure the indentation here is right. But also, variant should have an > encode overload that does this automatically. Why doesn’t this work? > > encoder << buffer; > > If it doesn't, we can fix that. I did not know about the encoder variant, will use it.
Comment on attachment 452843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452843&action=review >>> Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:46 >>> + , m_sharedVideoFrameReader(nullptr) >> >> This should be initialized in the header, not the .cpp file. > > This requires including RemoteVideoFrameObjectHeap.h in SharedVideoFrame.h header file. > I guess I can change the constructor to take a RemoteVideoFrameObjectHeap* instead of a RefPtr<RemoteVideoFrameObjectHeap>&&. Or add a default constructor.
Created attachment 452943 [details] Patch for landing
Committed r290358 (247676@main): <https://commits.webkit.org/247676@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452943 [details].
<rdar://problem/89343447>
(In reply to youenn fablet from comment #5) > Created attachment 452943 [details] > Patch for landing I forgot to commit the changes to use variant encoder before landing the patch... I will upload a follow-up patch to fix that.
Reopening to attach new patch.
Created attachment 452965 [details] Post-commit changes
Committed r290373 (247689@main): <https://commits.webkit.org/247689@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452965 [details].