Bug 169474

Summary: [MediaStream] Move paintCurrentFrameInContext from RealtimeMediaSources to MediaPlayer
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, jer.noble, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch.
youennf: review+, buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Update patch.
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Updated patch.
none
Updated patch. none

Description Eric Carlson 2017-03-10 09:51:05 PST
Every realtime video source has essentially identical copies of paintCurrentFrameInContext, move the logic to MediaPlayerPrivateMediaStreamAVFObjC.
Comment 1 Eric Carlson 2017-03-10 10:59:43 PST
Created attachment 304055 [details]
Proposed patch.
Comment 2 Radar WebKit Bug Importer 2017-03-10 11:00:21 PST
<rdar://problem/30976747>
Comment 3 youenn fablet 2017-03-10 11:15:31 PST
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?
Comment 4 Build Bot 2017-03-10 12:08:19 PST
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
Comment 5 Build Bot 2017-03-10 12:08:22 PST
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
Comment 6 Eric Carlson 2017-03-10 12:39:14 PST
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.
Comment 7 Eric Carlson 2017-03-13 07:15:17 PDT
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.
Comment 8 Eric Carlson 2017-03-13 07:16:23 PDT
Created attachment 304254 [details]
Update patch.

This is different enough from the first patch that it should be reviewed again.
Comment 9 Build Bot 2017-03-13 08:28:43 PDT
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
Comment 10 Build Bot 2017-03-13 08:28:47 PDT
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
Comment 11 youenn fablet 2017-03-13 09:08:52 PDT
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())
...
Comment 12 Eric Carlson 2017-03-13 13:53:28 PDT
Created attachment 304294 [details]
Updated patch.
Comment 13 youenn fablet 2017-03-13 15:19:47 PDT
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?
Comment 14 Eric Carlson 2017-03-13 16:27:41 PDT
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.
Comment 15 Eric Carlson 2017-03-13 16:28:38 PDT
Created attachment 304321 [details]
Updated patch.
Comment 16 WebKit Commit Bot 2017-03-13 17:30:52 PDT
Comment on attachment 304321 [details]
Updated patch.

Clearing flags on attachment: 304321

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