Bug 190519

<rdar://problem/45224307>

Created attachment 352200 [details] Patch

Comment on attachment 352200 [details] Patch Looks like a good refactoring! Some nits below. I haven't looked at why iOS/Mac are not compiling. View in context: https://bugs.webkit.org/attachment.cgi?id=352200&action=review > Source/WebCore/platform/MediaSample.h:96 > + virtual uint32_t videoPixelFormat() const { return 0; } It feels like it should be a MediaSampleObjC method only. > Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.h:67 > + uint32_t videoPixelFormat() const override; s/override/final/ > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:31 > +#include "MediaSample.h" forward declare MediaSample? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:32 > +#include <CoreMedia/CMTime.h> Do we need this one? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:51 > + line to remove? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:101 > +RetainPtr<CVPixelBufferRef> ImageTransferSessionVT::createPixelBuffer(CVPixelBufferRef sourceBuffer, const IntSize& size) s/createPixelBuffer/convertPixelBuffer/ ? Ditto for other methods. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:104 > + return nullptr; I wonder whether we should special case when sourceBuffer has the right size and right format. We should then be able to directly return sourceBuffer. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:131 > + return convertedPixelBuffer; Probably can be rewritten as: return createPixelBuffer(CMSampleBufferGetImageBuffer(sourceBuffer), size); Maybe sourceBuffer check is needed though? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:134 > +RetainPtr<CMSampleBufferRef> ImageTransferSessionVT::createCMSampleBuffer(CMSampleBufferRef sourceBuffer, const IntSize& size) s/createCMSampleBuffer/convertCMSampleBuffer/? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:137 > + return nullptr; Already done within next call to createPixelBuffer. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:143 > + CMVideoFormatDescriptionRef formatDescription = nullptr; Should we store formatDescription in a retain pointer to ensure we do not leak it in case of early return? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:185 > + if (!m_cgImageBufferPool || m_cgImagePoolSize != imageSize) { Do we need a m_cgImageBufferPool? It seems we will only use one ARGB pixel buffer at a time since we will convert it later on. Except maybe if we optimize the case of same-size same format case. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:200 > + CVReturn status = CVPixelBufferPoolCreate(kCFAllocatorDefault, pixelBufferPoolOptions, pixelBufferOptions, &bufferPool); auto > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:209 > + CVReturn status = CVPixelBufferPoolCreatePixelBuffer(kCFAllocatorDefault, m_cgImageBufferPool.get(), &rgbBuffer); auto > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:233 > + if (bufferSize != m_size) { If the format is not the right one, we should probably call createPixelBuffer. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:168 > + RefPtr<MediaSample> protectedMediaSample = &mediaSample; It seems we should only do this assignment when using m_imageTransferSession. > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:228 > + std::unique_ptr<ImageTransferSessionVT> m_imageTransferSession; I am not sure whether this should be added at that level, RealtimeMediaSource is an abstract class. Maybe we should keep it at the leaf classes for now. We might want to have it in RealtimeIncomingVideoSourceCocoa as well (this patch or another).

Created attachment 352514 [details] Patch

Comment on attachment 352200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352200&action=review >> Source/WebCore/platform/MediaSample.h:96 >> + virtual uint32_t videoPixelFormat() const { return 0; } > > It feels like it should be a MediaSampleObjC method only. Changed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:31 >> +#include "MediaSample.h" > > forward declare MediaSample? We do now. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:32 >> +#include <CoreMedia/CMTime.h> > > Do we need this one? No, removed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:51 >> + > > line to remove? Removed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:101 >> +RetainPtr<CVPixelBufferRef> ImageTransferSessionVT::createPixelBuffer(CVPixelBufferRef sourceBuffer, const IntSize& size) > > s/createPixelBuffer/convertPixelBuffer/ ? > > Ditto for other methods. Good point, changed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:104 >> + return nullptr; > > I wonder whether we should special case when sourceBuffer has the right size and right format. > We should then be able to directly return sourceBuffer. Yes, fixed this and convertCMSampleBuffer. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:131 >> + return convertedPixelBuffer; > > Probably can be rewritten as: > return createPixelBuffer(CMSampleBufferGetImageBuffer(sourceBuffer), size); > Maybe sourceBuffer check is needed though? Changed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:137 >> + return nullptr; > > Already done within next call to createPixelBuffer. Changed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:143 >> + CMVideoFormatDescriptionRef formatDescription = nullptr; > > Should we store formatDescription in a retain pointer to ensure we do not leak it in case of early return? I moved the call to CMVideoFormatDescriptionCreateForImageBuffer to just before it is used. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:185 >> + if (!m_cgImageBufferPool || m_cgImagePoolSize != imageSize) { > > Do we need a m_cgImageBufferPool? > It seems we will only use one ARGB pixel buffer at a time since we will convert it later on. > Except maybe if we optimize the case of same-size same format case. We probably don't need it, removed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:200 >> + CVReturn status = CVPixelBufferPoolCreate(kCFAllocatorDefault, pixelBufferPoolOptions, pixelBufferOptions, &bufferPool); > > auto Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:209 >> + CVReturn status = CVPixelBufferPoolCreatePixelBuffer(kCFAllocatorDefault, m_cgImageBufferPool.get(), &rgbBuffer); > > auto Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:233 >> + if (bufferSize != m_size) { > > If the format is not the right one, we should probably call createPixelBuffer. Good point, fixed. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:168 >> + RefPtr<MediaSample> protectedMediaSample = &mediaSample; > > It seems we should only do this assignment when using m_imageTransferSession. I moved this logic to the derived classes. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:228 >> + std::unique_ptr<ImageTransferSessionVT> m_imageTransferSession; > > I am not sure whether this should be added at that level, RealtimeMediaSource is an abstract class. > Maybe we should keep it at the leaf classes for now. > We might want to have it in RealtimeIncomingVideoSourceCocoa as well (this patch or another). Lets change RealtimeIncomingVideoSourceCocoa in a followup.

Created attachment 352544 [details] Patch

Attachment 352544 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:168: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:169: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:206: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 6 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 352545 [details] Patch

Attachment 352545 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 4 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 352546 [details] Patch

Attachment 352546 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 4 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 352570 [details] Patch

Attachment 352570 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 4 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 352570 [details] Patch It seems some of the minor changes could impact tests, for instance deviceID. View in context: https://bugs.webkit.org/attachment.cgi?id=352570&action=review > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:31 > +#include "MediaSampleAVFObjc.h" Is this one needed here? It is already included in the .mm > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55 > + RefPtr<MediaSample> createMediaSample(IOSurfaceRef, const MediaTime&, const IntSize&, MediaSample::VideoRotation rotation = MediaSample::VideoRotation::None, bool mirrored = false); s/rotation =/=/ here and above. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:59 > + WEBCORE_EXPORT ImageTransferSessionVT(uint32_t); explicit. Maybe add pixelFormat since this is not clear without. I wonder whether we should consider typing the pixelFormat like we do for ObjectIdentifier. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:62 > + VTSessionSetProperty(transferSession, kVTPixelTransferPropertyKey_ScalingMode, kVTScalingMode_Trim); Should we check status for all of these and return nullptr in create in case this fails? Or output some debug/release logging if failing? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:131 > + RELEASE_LOG(Media, "ImageTransferSessionVT::createPixelBuffer: CMSampleBufferGetImageBuffer returned nullptr"); No need for this release logging, error case will be logged in convertPixelBuffer. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:258 > + return createCMSampleBuffer(pixelBuffer.get(), sampleTime, size); There will be some redundant checks related to size. Not a big deal but maybe a helper function could be used to share the same code between the two createCMSampleBuffer variants? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:284 > + int32_t extendedBottom = roundUpToMacroblockMultiple(height) - height; auto as well. Maybe add a comment to explain this? > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:295 > + } I do not understand well this code, in particular why we have m_ioSurfaceBufferAttributes sometimes not having any kCVPixelBufferExtendedPixelsRightKey/kCVPixelBufferExtendedPixelsBottomKey > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:325 > + return nullptr; These checks are done within below createPixelBuffer call. > Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:343 > + return nullptr; Isn't setSize called within convertCMSampleBuffer? > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:106 > + void processNewFrame(RefPtr<MediaSample>); RefPtr<MediaSample>&& probably. Could we make it a Ref<MediaSample>&&? > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:564 > + scheduleDeferredTask([this, sample = WTFMove(sample)] { sample = sample.releaseNonNull() should do the trick to make it a Ref > Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:174 > + auto frame = generateFrame(); Can frame be null? If not, we can probably remove the if checks within the switchOn lambdas. > Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:85 > + return; Put this if inside the previous if. I wonder whether we need release logging here, maybe ImageTransferSessionVT::create is already doing that for us. > Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:90 > + return; Ditto here for logging. > Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:93 > + dispatchMediaSampleToObservers(*sampleBuffer.get()); s/.get()// ?

Created attachment 352576 [details] Patch

Attachment 352576 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:188: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 5 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 352596 [details] Patch for landing.

Attachment 352596 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:51: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:52: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55: The parameter name "rotation" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 4 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 352623 [details] patch for landing

Comment on attachment 352570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352570&action=review >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:31 >> +#include "MediaSampleAVFObjc.h" > > Is this one needed here? It is already included in the .mm An include is needed for the VideoRotation enum. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55 >> + RefPtr<MediaSample> createMediaSample(IOSurfaceRef, const MediaTime&, const IntSize&, MediaSample::VideoRotation rotation = MediaSample::VideoRotation::None, bool mirrored = false); > > s/rotation =/=/ here and above. Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:59 >> + WEBCORE_EXPORT ImageTransferSessionVT(uint32_t); > > explicit. Maybe add pixelFormat since this is not clear without. > I wonder whether we should consider typing the pixelFormat like we do for ObjectIdentifier. Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:62 >> + VTSessionSetProperty(transferSession, kVTPixelTransferPropertyKey_ScalingMode, kVTScalingMode_Trim); > > Should we check status for all of these and return nullptr in create in case this fails? > Or output some debug/release logging if failing? Logging added. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:284 >> + int32_t extendedBottom = roundUpToMacroblockMultiple(height) - height; > > auto as well. > Maybe add a comment to explain this? Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:325 >> + return nullptr; > > These checks are done within below createPixelBuffer call. Fixed. >> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:343 >> + return nullptr; > > Isn't setSize called within convertCMSampleBuffer? Indeed, removed. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:106 >> + void processNewFrame(RefPtr<MediaSample>); > > RefPtr<MediaSample>&& probably. > Could we make it a Ref<MediaSample>&&? Fixed. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:564 >> + scheduleDeferredTask([this, sample = WTFMove(sample)] { > > sample = sample.releaseNonNull() should do the trick to make it a Ref OK >> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:174 >> + auto frame = generateFrame(); > > Can frame be null? > If not, we can probably remove the if checks within the switchOn lambdas. Yes, it can be NULL. >> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:85 >> + return; > > Put this if inside the previous if. > I wonder whether we need release logging here, maybe ImageTransferSessionVT::create is already doing that for us. Fixed. >> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:90 >> + return; > > Ditto here for logging. createMediaSample will log in the event of a failure. >> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:93 >> + dispatchMediaSampleToObservers(*sampleBuffer.get()); > > s/.get()// ? Removed.

Attachment 352623 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:203: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 352623 [details] patch for landing Clearing flags on attachment: 352623 Committed r237236: <https://trac.webkit.org/changeset/237236>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 352660 [details] Fix iOSMac build

Comment on attachment 352660 [details] Fix iOSMac build Rejecting attachment 352660 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 352660, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/9644368

Comment on attachment 352660 [details] Fix iOSMac build Clearing flags on attachment: 352660 Committed r237240: <https://trac.webkit.org/changeset/237240>

All reviewed patches have been landed. Closing bug.