Bug 169474 – [MediaStream] Move paintCurrentFrameInContext from RealtimeMediaSources to MediaPlayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Every realtime video source has essentially identical copies of paintCurrentFrameInContext, move the logic to MediaPlayerPrivateMediaStreamAVFObjC.

[Eric Carlson]

Created attachment 304055 [details]
Proposed patch.

[Radar WebKit Bug Importer]

<rdar://problem/30976747>

[youenn fablet]

Comment on attachment 304055 [details]
Proposed patch.

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

> Source/WebCore/ChangeLog:10
> +        hang onto the most recent frame so it can implement paintCurrentFrameInContext directly.

I like that!

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:821
> +                        m_pixelBufferConformer = nullptr;

m_currentFrameImage, m_currentVideoFrameSample and m_currentVideoFrameSample are all used jointly and somehow in isolation from other members.
Might have been good (or maybe excessive?) to wrap all three parameters in a struct/class so that we would call something like imagePainter.clean()

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:905
> +        m_pixelBufferConformer = std::make_unique<PixelBufferConformerCV>((CFDictionaryRef)@{ (NSString *)kCVPixelBufferPixelFormatTypeKey: @(kCVPixelFormatType_32BGRA) });

This code will probably assert if VIDEOTOOLBOX is not enabled.
Maybe we should update PixelBufferConformerCV or adapt code here if VIDEOTOOLBOX is not enabled?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:911
> +    CVPixelBufferRef pixelBuffer = static_cast<CVPixelBufferRef>(CMSampleBufferGetImageBuffer(m_currentVideoFrameSample->platformSample().sample.cmSampleBuffer));

auto?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:914
> +    return;

unneeded return.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:923
> +    updateCurrentFrameImage();

Could we call updateCurrentFrameImage after the if?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:109
> +const OSType videoCaptureFormat = kCVPixelFormatType_420YpCbCr8BiPlanarFullRange;

kCVPixelFormatType_420YpCbCr8BiPlanarFullRange seems to be the best format to use.
Can we also use it on Mac platform?

[Build Bot]

Comment on attachment 304055 [details]
Proposed patch.

Attachment 304055 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3284777

New failing tests:
fast/mediastream/MediaStream-video-element-displays-buffer.html

[Build Bot]

Created attachment 304062 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Eric Carlson]

Comment on attachment 304055 [details]
Proposed patch.

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:821
>> +                        m_pixelBufferConformer = nullptr;
> 
> m_currentFrameImage, m_currentVideoFrameSample and m_currentVideoFrameSample are all used jointly and somehow in isolation from other members.
> Might have been good (or maybe excessive?) to wrap all three parameters in a struct/class so that we would call something like imagePainter.clean()

Good idea, fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:905
>> +        m_pixelBufferConformer = std::make_unique<PixelBufferConformerCV>((CFDictionaryRef)@{ (NSString *)kCVPixelBufferPixelFormatTypeKey: @(kCVPixelFormatType_32BGRA) });
> 
> This code will probably assert if VIDEOTOOLBOX is not enabled.
> Maybe we should update PixelBufferConformerCV or adapt code here if VIDEOTOOLBOX is not enabled?

We won't be able to paint some formats if VIDEOTOOLBOX is not enabled, so I think asserting here is the right thing to do.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:911
>> +    CVPixelBufferRef pixelBuffer = static_cast<CVPixelBufferRef>(CMSampleBufferGetImageBuffer(m_currentVideoFrameSample->platformSample().sample.cmSampleBuffer));
> 
> auto?

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:914
>> +    return;
> 
> unneeded return.

Removed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:923
>> +    updateCurrentFrameImage();
> 
> Could we call updateCurrentFrameImage after the if?

No, because it updates the current image if necessary.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:109
>> +const OSType videoCaptureFormat = kCVPixelFormatType_420YpCbCr8BiPlanarFullRange;
> 
> kCVPixelFormatType_420YpCbCr8BiPlanarFullRange seems to be the best format to use.
> Can we also use it on Mac platform?

Good idea, but using kCVPixelFormatType_420YpCbCr8BiPlanarFullRange causes problems with AVSampleBufferDisplayLayer that will require more code to work around, so I will do this in a followup patch.

[Eric Carlson]

The failure in MediaStream-video-element-displays-buffer.html was because of an existing race condition made worse by these changes. That tests calls play in the 'canplay' event handler, and paints the <video> element to a canvas in the 'playing' event handlers which assumes a video frame is available to draw as soon as the readyState reaches 'have enough data'. 

Even though the readyState advanced as soon as there was an active video track, this usually worked because the video realtime capture source was responsible for painting. With the changes in this patch it frequently failed because the media player was responsible for painting but it didn't always have a cached video sample by the time paint was called. 

Fix this by adding a readyState to MediaStreamTrackPrivate and blocking the video element readyState on that.

[Eric Carlson]

Created attachment 304254 [details]
Update patch.

This is different enough from the first patch that it should be reviewed again.

[Build Bot]

Comment on attachment 304254 [details]
Update patch.

Attachment 304254 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3307863

New failing tests:
fast/mediastream/MediaStream-video-element-video-tracks-disabled-then-enabled.html

[Build Bot]

Created attachment 304257 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 304254 [details]
Update patch.

A test is failing so not doing a full review.
I like the direction although I may not see all consequences of all changes.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:922
> +    if (m_displayMode == PaintItBlack || !m_imagePainter.cgImage || !m_imagePainter.mediaSample) {

It would be good to not call updateCurrentFrameImage()  if (m_displayMode == PaintItBlack).
I guess we could write something like:
 if (m_displayMode == PaintItBlack || !updateCurrentFrameImage())
...

[Eric Carlson]

Created attachment 304294 [details]
Updated patch.

[youenn fablet]

Comment on attachment 304294 [details]
Updated patch.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:81
> +    void layerStatusDidChange(AVSampleBufferDisplayLayer*);

Can layer be null?

> Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm:351
> +    m_callback = 0;

Can we use nullptr?

[Eric Carlson]

Comment on attachment 304294 [details]
Updated patch.

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:81
>> +    void layerStatusDidChange(AVSampleBufferDisplayLayer*);
> 
> Can layer be null?

No.

>> Source/WebCore/platform/mediastream/mac/AVMediaCaptureSource.mm:351
>> +    m_callback = 0;
> 
> Can we use nullptr?

Fixed.

[Eric Carlson]

Created attachment 304321 [details]
Updated patch.

[WebKit Commit Bot]

Comment on attachment 304321 [details]
Updated patch.

Clearing flags on attachment: 304321

Committed r213880: <http://trac.webkit.org/changeset/213880>