Bug 125157 - [MSE][Mac] Support painting MSE video-element to canvas
Summary: [MSE][Mac] Support painting MSE video-element to canvas
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
: 154189 (view as bug list)
Depends on: 154544
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-03 08:53 PST by Jer Noble
Modified: 2017-03-21 16:36 PDT (History)
16 users (show)

See Also:


Attachments
Patch (124.64 KB, patch)
2016-02-29 11:29 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (124.72 KB, patch)
2016-03-01 08:45 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (124.73 KB, patch)
2016-03-01 09:17 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.38 MB, application/zip)
2016-03-01 10:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-03-01 10:06 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.35 MB, application/zip)
2016-03-01 10:20 PST, Build Bot
no flags Details
Patch (125.87 KB, patch)
2016-03-01 11:13 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (126.03 KB, patch)
2016-03-02 08:27 PST, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch rebased against ToT (96.18 KB, patch)
2016-12-08 10:43 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch rebased against ToT (121.05 KB, patch)
2016-12-08 10:49 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (8.47 KB, patch)
2017-03-21 16:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-12-03 08:53:24 PST
[MSE][Mac] Support painting MSE video-element to canvas
Comment 1 Radar WebKit Bug Importer 2015-10-10 19:06:45 PDT
<rdar://problem/23062016>
Comment 2 Alexey Proskuryakov 2015-11-19 09:02:10 PST
What is the proper way to track cross-origin/tainting in this case?
Comment 3 Jer Noble 2015-11-19 09:04:08 PST
For a first pass, we would have to assume that all painting from MSE taints the canvas.

Second pass, taint the canvas if any non-single-origin XHR requests were made.

Third pass, attach the single-origin state to the ArrayBuffer generated by XHRs, and ferry that data through MSE to the video element.
Comment 4 Jer Noble 2016-02-12 15:36:42 PST
*** Bug 154189 has been marked as a duplicate of this bug. ***
Comment 5 Daniel Rossi 2016-02-13 14:55:34 PST
This is a matter of leaving things to the last minute. An accident waiting to happen. 

It is related to the CORS flaw. Once CORS support is enabled , MediaSource WebGL textures will still be a problem. A very basic usage. 

See this for reference

https://bugs.webkit.org/show_bug.cgi?id=135379

My proxy hack work around does not work on Dash only mp4 files.
Comment 6 Daniel Rossi 2016-02-13 14:56:47 PST
CORS is needed because in the real world people use CDN's for video with sub domain cnames which break CORS.
Comment 7 Daniel Rossi 2016-02-13 20:02:51 PST
I have left a comment here. These two are related and nobody is assigned to the actual CORS flaw issue

https://bugs.webkit.org/show_bug.cgi?id=135379#c43
Comment 8 Daniel Rossi 2016-02-15 04:03:36 PST
I got myself confused. It seems when drawing to 2D canvas there is no CORS restriction it obviously does not use CORS. It's only when you attempt to get a data uri to get an image from the video does CORS become a problem which requires the proxy hack. 

However when trying to use WebGL there is and because Safari lacks CORS support , we have this issue. 

Here is a raw webgl example just with an mp4 file. 

https://jsfiddle.net/7t77rz6L/11/

"SecurityError: DOM Exception 18: An attempt was made to break through the security policy of the user agent."
Comment 9 Daniel Rossi 2016-02-15 06:43:48 PST
Just confirming even with the proxy hack, Iphone will not play inline. It needs serious dodgy hacks of updating the loaded video time and separate audio. No good for webgl / VR needing to display the canvas. 

That same proxy hack to work around CORS does not work on MediaSource however.
Comment 10 Jer Noble 2016-02-29 11:29:46 PST
Created attachment 272504 [details]
Patch
Comment 11 WebKit Commit Bot 2016-02-29 11:31:05 PST
Attachment 272504 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Daniel Rossi 2016-03-01 00:53:54 PST
That dependancy bug is fixed. Does that means CORS and webgl will play nicely soon ?  

Absolutely everyone in VR is depending on this minus the Iphone inline issue, not sure how to tackle that one, some nasty hacks out there. 

Apple can't market VR until this is done really but Iphone still unusable.
Comment 13 Jer Noble 2016-03-01 08:42:08 PST
(In reply to comment #12)
> That dependancy bug is fixed. Does that means CORS and webgl will play
> nicely soon ?  

This bug will only address the painting to canvas use case. There is still more work to enable efficient rendering to WebGL.
Comment 14 Jer Noble 2016-03-01 08:45:51 PST
Created attachment 272565 [details]
Patch
Comment 15 WebKit Commit Bot 2016-03-01 08:47:46 PST
Attachment 272565 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Jer Noble 2016-03-01 09:17:29 PST
Created attachment 272566 [details]
Patch
Comment 17 WebKit Commit Bot 2016-03-01 09:18:49 PST
Attachment 272566 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2016-03-01 10:02:39 PST
Comment on attachment 272566 [details]
Patch

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

New failing tests:
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
http/tests/media/media-source/mediasource-remove.html
media/media-source/media-source-stalled-holds-sleep-assertion.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-paint-to-canvas.html
Comment 19 Build Bot 2016-03-01 10:02:42 PST
Created attachment 272568 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Build Bot 2016-03-01 10:06:25 PST
Comment on attachment 272566 [details]
Patch

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

New failing tests:
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
http/tests/media/media-source/mediasource-remove.html
media/media-source/media-source-stalled-holds-sleep-assertion.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-paint-to-canvas.html
Comment 21 Build Bot 2016-03-01 10:06:29 PST
Created attachment 272570 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 22 Jer Noble 2016-03-01 10:11:50 PST
Crashes are an ASSERT in initVTDecompressionSessionDecodeFrameWithOutputHandler(...). Looking at the headers, I see __OSX_AVAILABLE_STARTING(__MAC_10_11,__IPHONE_9_0);

Looks like I'll need to move things around to use VTDecompressionSessionDecodeFrame instead.
Comment 23 Build Bot 2016-03-01 10:19:59 PST
Comment on attachment 272566 [details]
Patch

Attachment 272566 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/906852

New failing tests:
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
http/tests/media/media-source/mediasource-remove.html
media/media-source/media-source-stalled-holds-sleep-assertion.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-paint-to-canvas.html
Comment 24 Build Bot 2016-03-01 10:20:03 PST
Created attachment 272572 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 25 Jer Noble 2016-03-01 11:13:35 PST
Created attachment 272576 [details]
Patch
Comment 26 Jer Noble 2016-03-02 08:27:25 PST
Created attachment 272657 [details]
Patch
Comment 27 Eric Carlson 2016-03-02 09:44:48 PST
Comment on attachment 272657 [details]
Patch

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

r=me wth some nits and restructuring suggestions.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:580
> +        m_player->firstVideoFrameAvailable();

Do you want to call firstVideoFrameAvailable more than once in the player's lifecycle?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:611
> +class WebCoreDecompressionSession : public ThreadSafeRefCounted<WebCoreDecompressionSession> {
> +public:
> +    static Ref<WebCoreDecompressionSession> create(AVSampleBufferDisplayLayer* layer, AVSampleBufferRenderSynchronizer *synchronizer) { return adoptRef(*new WebCoreDecompressionSession(layer, synchronizer)); }

Is there any reason to hide this here in SourceBufferPrivateAVFObjC.mm? It doesn't look like this class is MSE-specific in any way, and it could be extremely useful for other media engines (eg. MediaStream).

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:650
> +    std::function<void()> m_notificationCallback;

Nit: This name is really generic for a callback with a specific purpose. Maybe m_requestMediaDataCallback?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:700
> +    timingInfoArray.grow(itemCount);

Why .grow() instead of .reserveCapacity(), or  Vector<CMSampleTimingInfo> vector(itemCount)?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:720
> +        CMBufferQueueCreate(kCFAllocatorDefault, kMaximumCapacity, &callbacks, &outQueue);

Nit: kMaximumQueueCapacity would be clearer.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:723
> +        CMBufferQueueInstallTriggerWithIntegerThreshold(m_producerQueue.get(), maybeBecomeReadyForMoreMediaDataCallback, this, kCMBufferQueueTrigger_WhenBufferCountBecomesLessThan, 15, &m_didBecomeReadyTrigger);

This magic number should be a named constant to make it easier to understand what it means.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:760
> +    if (m_decompressionSession && !VTDecompressionSessionCanAcceptFormatDescription(m_decompressionSession.get(), videoFormatDescription)) {
> +        VTDecompressionSessionWaitForAsynchronousFrames(m_decompressionSession.get());
> +        m_decompressionSession = nullptr;
> +    }

Nit: you should definitely add error logging here to make it easier to diagnose failures.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770
> +        VTDecompressionOutputCallbackRecord callback {
> +            &decompressionOutputCallback,
> +            this,
> +        };

What guarantees that "this" stays alive? Should it be a weakPtr?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:794
> +    if (noErr != CMVideoFormatDescriptionCreateForImageBuffer(kCFAllocatorDefault, rawImageBuffer, &rawImageBufferDescription))
> +        return;

You should log this error.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:805
> +    if (noErr != CMSampleBufferCreateForImageBuffer(kCFAllocatorDefault, rawImageBuffer, true, nullptr, nullptr, imageBufferDescription.get(), &imageBufferTiming, &rawImageSampleBuffer))
> +        return;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:809
> +    callOnMainThread([strongThis, status, imageSampleBuffer, infoFlags, displaying] {
> +        UNUSED_PARAM(infoFlags);

Why pass infoFlags if you don't use them?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:819
> +    --m_framesBeingDecoded;

ASSERT(m_framesBeingDecoded >= 0)

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:858
> +        RetainPtr<CMSampleBufferRef> sample = adoptCF((CMSampleBufferRef)CMBufferQueueDequeueAndRetain(m_producerQueue.get()));
> +        [m_displayLayer enqueueSampleBuffer:sample.get()];
> +        CMBufferQueueEnqueue(m_consumerQueue.get(), sample.get());

Is this correct if [m_displayLayer isReadyForMoreMediaData] above returned false?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:935
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);
> +    return CMSampleBufferGetDecodeTimeStamp(sample);

Nit: you don't need the local variable.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:942
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);
> +    return CMSampleBufferGetPresentationTimeStamp(sample);

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:949
> +    CMSampleBufferRef sample = (CMSampleBufferRef)(buf);
> +    return CMSampleBufferGetDuration(sample);

Ditto.
Comment 28 Jer Noble 2016-03-02 10:49:30 PST
Comment on attachment 272657 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:580
>> +        m_player->firstVideoFrameAvailable();
> 
> Do you want to call firstVideoFrameAvailable more than once in the player's lifecycle?

Yes.  This lets the media element know to switch from displaying the poster frame to displaying the video.  And this is not limited to happening only once per player.  If this player is seeked out of it's buffered range, it could not have flushed all its images and have nothing to display, in which case the media element will have switched back to displaying the poster. Perhaps we need to rename the client method though.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:611
>> +    static Ref<WebCoreDecompressionSession> create(AVSampleBufferDisplayLayer* layer, AVSampleBufferRenderSynchronizer *synchronizer) { return adoptRef(*new WebCoreDecompressionSession(layer, synchronizer)); }
> 
> Is there any reason to hide this here in SourceBufferPrivateAVFObjC.mm? It doesn't look like this class is MSE-specific in any way, and it could be extremely useful for other media engines (eg. MediaStream).

This could get pulled into its own file, sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:650
>> +    std::function<void()> m_notificationCallback;
> 
> Nit: This name is really generic for a callback with a specific purpose. Maybe m_requestMediaDataCallback?

Sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:700
>> +    timingInfoArray.grow(itemCount);
> 
> Why .grow() instead of .reserveCapacity(), or  Vector<CMSampleTimingInfo> vector(itemCount)?

The latter isn't possible; you need to pass in a `const CMSampleTimingInfo&` in addition to a itemCount, and setting specific values is unnecessary since the next method will overwrite the contents anyway. And while the former resizes the Vector's buffer, it doesn't resize the vector itself.

Of course, now that I read this, I can't remember why I needed the sample timing at all. I'm pretty sure I can drop this whole section.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:720
>> +        CMBufferQueueCreate(kCFAllocatorDefault, kMaximumCapacity, &callbacks, &outQueue);
> 
> Nit: kMaximumQueueCapacity would be clearer.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:723
>> +        CMBufferQueueInstallTriggerWithIntegerThreshold(m_producerQueue.get(), maybeBecomeReadyForMoreMediaDataCallback, this, kCMBufferQueueTrigger_WhenBufferCountBecomesLessThan, 15, &m_didBecomeReadyTrigger);
> 
> This magic number should be a named constant to make it easier to understand what it means.

You're right, this is supposed to be kLowWaterMark.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:760
>> +    }
> 
> Nit: you should definitely add error logging here to make it easier to diagnose failures.

This is definitely not an error condition.  If the client starts pushing in different dimension samples, we may need to restart the decompression session.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770
>> +        };
> 
> What guarantees that "this" stays alive? Should it be a weakPtr?

Unfortunately, it can't be.  It has to be a bare pointer.  However, we're not using asynchronous decoding flags when we decode frames, so the callback will occur before VTDecompressionSessionDecodeFrame() returns. So there's no danger here.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:794
>> +        return;
> 
> You should log this error.

ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:805
>> +        return;
> 
> Ditto.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:809
>> +        UNUSED_PARAM(infoFlags);
> 
> Why pass infoFlags if you don't use them?

Good point. :)

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:819
>> +    --m_framesBeingDecoded;
> 
> ASSERT(m_framesBeingDecoded >= 0)

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:858
>> +        CMBufferQueueEnqueue(m_consumerQueue.get(), sample.get());
> 
> Is this correct if [m_displayLayer isReadyForMoreMediaData] above returned false?

This won't be reached if -isReadyForMoreMediaData returns false.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:935
>> +    return CMSampleBufferGetDecodeTimeStamp(sample);
> 
> Nit: you don't need the local variable.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:942
>> +    return CMSampleBufferGetPresentationTimeStamp(sample);
> 
> Ditto.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:949
>> +    return CMSampleBufferGetDuration(sample);
> 
> Ditto.

Ok.
Comment 29 Jer Noble 2016-03-02 10:49:32 PST
Comment on attachment 272657 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:580
>> +        m_player->firstVideoFrameAvailable();
> 
> Do you want to call firstVideoFrameAvailable more than once in the player's lifecycle?

Yes.  This lets the media element know to switch from displaying the poster frame to displaying the video.  And this is not limited to happening only once per player.  If this player is seeked out of it's buffered range, it could not have flushed all its images and have nothing to display, in which case the media element will have switched back to displaying the poster. Perhaps we need to rename the client method though.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:611
>> +    static Ref<WebCoreDecompressionSession> create(AVSampleBufferDisplayLayer* layer, AVSampleBufferRenderSynchronizer *synchronizer) { return adoptRef(*new WebCoreDecompressionSession(layer, synchronizer)); }
> 
> Is there any reason to hide this here in SourceBufferPrivateAVFObjC.mm? It doesn't look like this class is MSE-specific in any way, and it could be extremely useful for other media engines (eg. MediaStream).

This could get pulled into its own file, sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:650
>> +    std::function<void()> m_notificationCallback;
> 
> Nit: This name is really generic for a callback with a specific purpose. Maybe m_requestMediaDataCallback?

Sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:700
>> +    timingInfoArray.grow(itemCount);
> 
> Why .grow() instead of .reserveCapacity(), or  Vector<CMSampleTimingInfo> vector(itemCount)?

The latter isn't possible; you need to pass in a `const CMSampleTimingInfo&` in addition to a itemCount, and setting specific values is unnecessary since the next method will overwrite the contents anyway. And while the former resizes the Vector's buffer, it doesn't resize the vector itself.

Of course, now that I read this, I can't remember why I needed the sample timing at all. I'm pretty sure I can drop this whole section.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:720
>> +        CMBufferQueueCreate(kCFAllocatorDefault, kMaximumCapacity, &callbacks, &outQueue);
> 
> Nit: kMaximumQueueCapacity would be clearer.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:723
>> +        CMBufferQueueInstallTriggerWithIntegerThreshold(m_producerQueue.get(), maybeBecomeReadyForMoreMediaDataCallback, this, kCMBufferQueueTrigger_WhenBufferCountBecomesLessThan, 15, &m_didBecomeReadyTrigger);
> 
> This magic number should be a named constant to make it easier to understand what it means.

You're right, this is supposed to be kLowWaterMark.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:760
>> +    }
> 
> Nit: you should definitely add error logging here to make it easier to diagnose failures.

This is definitely not an error condition.  If the client starts pushing in different dimension samples, we may need to restart the decompression session.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:770
>> +        };
> 
> What guarantees that "this" stays alive? Should it be a weakPtr?

Unfortunately, it can't be.  It has to be a bare pointer.  However, we're not using asynchronous decoding flags when we decode frames, so the callback will occur before VTDecompressionSessionDecodeFrame() returns. So there's no danger here.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:794
>> +        return;
> 
> You should log this error.

ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:805
>> +        return;
> 
> Ditto.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:809
>> +        UNUSED_PARAM(infoFlags);
> 
> Why pass infoFlags if you don't use them?

Good point. :)

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:819
>> +    --m_framesBeingDecoded;
> 
> ASSERT(m_framesBeingDecoded >= 0)

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:858
>> +        CMBufferQueueEnqueue(m_consumerQueue.get(), sample.get());
> 
> Is this correct if [m_displayLayer isReadyForMoreMediaData] above returned false?

This won't be reached if -isReadyForMoreMediaData returns false.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:935
>> +    return CMSampleBufferGetDecodeTimeStamp(sample);
> 
> Nit: you don't need the local variable.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:942
>> +    return CMSampleBufferGetPresentationTimeStamp(sample);
> 
> Ditto.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:949
>> +    return CMSampleBufferGetDuration(sample);
> 
> Ditto.

Ok.
Comment 30 Daniel Rossi 2016-04-06 02:27:09 PDT
Any ideas of the status of this ? I am hoping this means there is CORS support in webkit after this ?
Comment 31 Daniel Rossi 2016-05-13 04:02:38 PDT
Hi guys this has gone quiet. VR people are waiting for this fix and it's pretty critical. Already missed the boat it seems ! 

I'm going to see if running in WebView / Cordova works around this lack of implementation like it can override inline restrictions.
Comment 32 Daniel Rossi 2016-06-03 08:15:06 PDT
Any ideas of the status of this ? 

I know it's pretty much crept up and steamrolled, many people trying to do WebVR including Youtube and Facebook of all people are also waiting. 

They should pay to make this happen. 

This problem has been left exposed for many years now and was just an accident waiting to happen. For some reason a few of us have to do the run around for many others. 

Obviously the reverse proxy work around is not ok for production so not even Facebook or Youtube are doing that and therefore Safari is broken. 

If there is a nightly fix to test please let me know.
Comment 33 Alexandre Buisine 2016-06-03 08:22:24 PDT
I am interested as well in any experimental version and would be happy to run a bunch of test on it ! :)
Comment 34 Alexey Proskuryakov 2016-06-03 09:56:06 PDT
Please see <https://bugzilla.mozilla.org/page.cgi?id=etiquette.html>.
Comment 35 Dean Jackson 2016-06-03 10:04:49 PDT
Sorry, no progress to report on this bug at the moment.
Comment 36 Daniel Rossi 2016-06-14 18:58:58 PDT
Hi guys I'm sorry to use this as a forum. But by stealth it seems Apple have forked the code and fixed it correct ?

This is not so useful for current browsers so wondering if they will merge it back ? 

https://twitter.com/zenoc/status/742770789880111104
Comment 37 Dean Jackson 2016-06-15 10:29:48 PDT
(In reply to comment #36)
> Hi guys I'm sorry to use this as a forum. But by stealth it seems Apple have
> forked the code and fixed it correct ?
> 
> This is not so useful for current browsers so wondering if they will merge
> it back ? 
> 
> https://twitter.com/zenoc/status/742770789880111104

This only applies to videos on macOS running Sierra.
Comment 38 Daniel Rossi 2016-09-22 03:24:51 PDT
Yes I am aware they are only interested in fixing it in an OS nobody even has yet. Which renders all other OS useless. 

However I've discovered a quiet change in Safari in Yosemite which has broken feature detection. 

Something truly bizarre has changed in Safari on Yosemite that was not there before. It has broken feature detection with CORS in alot of things I have done. 

Just a heads up people just working how broken it really is. 

```
var testVideo = document.createElement("video");
 testVideo.crossOrigin = "anonymous";
console.log(testVideo.hasAttribute("crossOrigin"));

```

In IE11 it returns false as expected. In Safari it's true but the CORS bug is still obviously there. The feature detection is there to obviously require the proxy hack. Madness. 

So I can't help to think some half baked change was pushed ?
Comment 39 Jer Noble 2016-12-08 10:43:39 PST
Created attachment 296534 [details]
Patch rebased against ToT
Comment 40 Jer Noble 2016-12-08 10:49:30 PST
Created attachment 296535 [details]
Patch rebased against ToT
Comment 41 Jon Lee 2017-03-03 14:56:16 PST
rdar://problem/22925644
Comment 42 Jer Noble 2017-03-21 16:36:01 PDT
Created attachment 305047 [details]
Patch for landing