Bug 230623 - MediaStream canvas.captureStream() playback to LocalSampleBufferDisplayLayer is flaky
Summary: MediaStream canvas.captureStream() playback to LocalSampleBufferDisplayLayer ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 235176 (view as bug list)
Depends on: 230613
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-22 06:25 PDT by Kimmo Kinnunen
Modified: 2022-01-25 10:42 PST (History)
9 users (show)

See Also:


Attachments
testcase (1.27 KB, text/html)
2022-01-21 02:09 PST, Kimmo Kinnunen
no flags Details
Patch (5.50 KB, patch)
2022-01-21 03:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2022-01-21 09:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-09-22 06:25:51 PDT
MediaStream canvas.captureStream() playback to LocalSampleBufferDisplayLayer is flaky

Sometimes the system is not creating the AVSampleBufferDisplayLayer because the samples arrive 
before the media player determines whether accelerated rendering is possible.

The callbacks that update HTMLMediaElement::m_cachedSupportsAcceleratedRendering are run asynchronously and too late.
Comment 1 Kimmo Kinnunen 2021-09-22 06:25:58 PDT
To observe,  bug 230613 should be fixed and load:

https://webrtc.github.io/samples/src/content/capture/canvas-video/
Comment 2 Radar WebKit Bug Importer 2021-09-29 06:26:14 PDT
<rdar://problem/83668578>
Comment 3 Kimmo Kinnunen 2022-01-20 23:13:40 PST
*** Bug 235176 has been marked as a duplicate of this bug. ***
Comment 4 Kimmo Kinnunen 2022-01-21 02:09:07 PST
Created attachment 449646 [details]
testcase
Comment 5 Kimmo Kinnunen 2022-01-21 02:10:07 PST
To work around this problem, one might try to assign or re-assign the stream in a different JavaScript callback than the one that creates the video element.
Comment 6 youenn fablet 2022-01-21 03:22:22 PST
Created attachment 449649 [details]
Patch
Comment 7 youenn fablet 2022-01-21 09:08:45 PST
Created attachment 449663 [details]
Patch
Comment 8 Kimmo Kinnunen 2022-01-24 02:24:02 PST
Comment on attachment 449663 [details]
Patch

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

It is a step forward for the users, which is good.
The reenqueue logic is broken in many ways and not very good pattern for correct software and the class has numerous race conditions. We should spend some more time writing tests and fixing those for this class, at some point.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:284
> +    Locker locker { AdoptLock, m_currentVideoSampleLock };

This is like saying  "Try to send the sample. if it's a bit inconvenient, maybe you can just skip sending it."
Either the sample is needed for correct operation and send it or it's not needed and it should not be sent.
This  especially  does not make sense in this "reenqueue" phase. 
So you can consider using unconditional locking.
Comment 9 youenn fablet 2022-01-24 03:29:26 PST
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:284
> > +    Locker locker { AdoptLock, m_currentVideoSampleLock };
> 
> This is like saying  "Try to send the sample. if it's a bit inconvenient,
> maybe you can just skip sending it."

Not really, the idea is more: if it is locked, a new sample is coming, so do not bother enqueuing a sample, since a fresh one will be enqueued very shortly.
It is true that, sometimes, we will reenqueue several times the same sample.

Also we need to lock as m_currentVideoSample is guarded by m_currentVideoSampleLock.
Comment 10 EWS 2022-01-24 04:42:57 PST
Committed r288435 (246324@main): <https://commits.webkit.org/246324@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449663 [details].