[webkit-reviews] review granted: [Bug 125157] [MSE][Mac] Support painting MSE video-element to canvas : [Attachment 306734] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 125157: [MSE][Mac] Support painting MSE video-element to canvas
https://bugs.webkit.org/show_bug.cgi?id=125157

Attachment 306734: Patch

https://bugs.webkit.org/attachment.cgi?id=306734&action=review




--- Comment #66 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 306734
  --> https://bugs.webkit.org/attachment.cgi?id=306734
Patch

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

> Source/WebCore/ChangeLog:108
> +	   * platform/cocoa/VideoToolboxSoftLink.cpp: Added.
> +	   * platform/cocoa/VideoToolboxSoftLink.h: Added.
> +
> +2016-11-29  Jer Noble  <jer.noble at apple.com>
> +
> +	   [MSE][Mac] Support painting MSE video-element to canvas
> +	   https://bugs.webkit.org/show_bug.cgi?id=125157
> +	   <rdar://problem/23062016>

Double ChangeLog.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:538
> +    RetainPtr<CVPixelBufferRef> pixelBuffer =
m_decompressionSession->imageForTime(currentMediaTime(), flags);

auto?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:871
> +	   if (m_decompressionSession) {
> +	       m_decompressionSession->flush();
> +	      
m_decompressionSession->notifyWhenHasAvailableVideoFrame([weakThis =
createWeakPtr()] {
> +		   if (weakThis && weakThis->m_mediaSource)
> +		      
weakThis->m_mediaSource->player()->setHasAvailableVideoFrame(true);
> +	       });
> +	   }

Nit: trackDidChangeEnabled has the same code. Split into a helper function?

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:51
> +    auto weakThis = createWeakPtr();

Is this still necessary?

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:64
> +    WebCoreDecompressionSession* session =
static_cast<WebCoreDecompressionSession*>(refcon);
> +    session->maybeBecomeReadyForMoreMediaData();

Nit: the local variable isn't necessary.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:109
> +	  
CMBufferQueueInstallTriggerWithIntegerThreshold(m_producerQueue.get(),
maybeBecomeReadyForMoreMediaDataCallback, this,
kCMBufferQueueTrigger_WhenBufferCountBecomesLessThan, 15,
&m_didBecomeReadyTrigger);

Why "15", is it kHighWaterMark/2. A named constant would be nice.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:149
> +	   CMVideoFormatDescriptionRef videoFormatDescription =
CMSampleBufferGetFormatDescription(sample);

auto?

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:165
> +    VTDecompressionSessionDecodeFrame(m_decompressionSession.get(), sample,
0, reinterpret_cast<void*>(displaying), &flags);

An enum would be easier to follow than a bool.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:180
> +    RetainPtr<CMVideoFormatDescriptionRef> imageBufferDescription =
adoptCF(rawImageBufferDescription);

auto?

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:204
> +    RetainPtr<CMSampleBufferRef> currentSample =
adoptCF((CMSampleBufferRef)CMBufferQueueDequeueAndRetain(m_producerQueue.get())
);

Ditto.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:215
> +    --m_framesBeingDecoded;

ASSERT(m_framesBeingDecoded >= 0)

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:240
> +void
WebCoreDecompressionSession::requestMediaDataWhenReady(std::function<void()>
notificationCallback)

It doesn't look like notificationCallback is ever null, so
std::function<void()>&& notificationCallback

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:242
> +    m_notificationCallback = notificationCallback;

and m_notificationCallback = WTFMove(notificationCallback);

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:258
> +void
WebCoreDecompressionSession::notifyWhenHasAvailableVideoFrame(std::function<voi
d()> callback)

Ditto.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:261
> +	   dispatch_async(dispatch_get_main_queue(), [callback] {

dispatch_async(dispatch_get_main_queue(), [callback = WTFMove(callback)] {

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:266
> +    m_hasAvailableFrameCallback = callback;

m_hasAvailableFrameCallback = WTFMove(callback)

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:278
> +    MediaTime startTime =
toMediaTime(CMBufferQueueGetFirstDecodeTimeStamp(m_producerQueue.get()));
> +    MediaTime endTime =
toMediaTime(CMBufferQueueGetEndPresentationTimeStamp(m_producerQueue.get()));

auto?

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:287
> +    while (CMSampleBufferRef firstSample =
(CMSampleBufferRef)CMBufferQueueGetHead(m_producerQueue.get())) {
> +	   MediaTime presentationTimestamp =
toMediaTime(CMSampleBufferGetPresentationTimeStamp(firstSample));
> +	   MediaTime duration =
toMediaTime(CMSampleBufferGetDuration(firstSample));

Ditto.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:297
> +	   RetainPtr<CMSampleBufferRef> currentSample =
adoptCF((CMSampleBufferRef)CMBufferQueueDequeueAndRetain(m_producerQueue.get())
);
> +	   RetainPtr<CVPixelBufferRef> imageBuffer =
(CVPixelBufferRef)CMSampleBufferGetImageBuffer(currentSample.get());

Ditto.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:317
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);

Nit: local isn't necessary.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:324
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);

Ditto.

> Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm:331
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);

Ditto.