Bug 206286 - Add support for MediaStream video track rendering in GPUProcess
Summary: Add support for MediaStream video track rendering in GPUProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 206066
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-15 05:42 PST by youenn fablet
Modified: 2020-01-20 02:39 PST (History)
10 users (show)

See Also:


Attachments
Patch (79.83 KB, patch)
2020-01-15 05:55 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (96.63 KB, patch)
2020-01-16 02:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (97.30 KB, patch)
2020-01-16 02:54 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (98.97 KB, patch)
2020-01-16 05:01 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (100.37 KB, patch)
2020-01-16 08:15 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (96.07 KB, patch)
2020-01-20 00:50 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 youenn fablet 2020-01-15 05:42:30 PST
Add support for MediaStream video track rendering in GPUProcess
Comment 1 youenn fablet 2020-01-15 05:55:02 PST
Created attachment 387783 [details]
Patch
Comment 2 youenn fablet 2020-01-15 06:29:37 PST
Rendering does not work 100%, video frame is only updated when a full refresh is needed, like a window resize.
Comment 3 Eric Carlson 2020-01-15 08:59:18 PST
Comment on attachment 387783 [details]
Patch

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

> Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:138
> +    // This is only an approximation, maybe we should use a clock as well.
> +    return m_mediaTime;

I think we will need to use a clock, or time will "stop" between samples. Maybe use the wall clock to calculate the delta between samples?

> Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.messages.in:1
> +# Copyright (C) 2019 Apple Inc. All rights reserved.

2020

> Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.messages.in:1
> +# Copyright (C) 2019 Apple Inc. All rights reserved.

Ditto

> Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.messages.in:1
> +# Copyright (C) 2019 Apple Inc. All rights reserved.

Ditto

> Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayerIdentifier.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

Ditto
Comment 4 youenn fablet 2020-01-16 02:28:29 PST
Created attachment 387900 [details]
Patch
Comment 5 youenn fablet 2020-01-16 02:54:31 PST
Created attachment 387901 [details]
Patch
Comment 6 youenn fablet 2020-01-16 05:01:11 PST
Created attachment 387910 [details]
Patch
Comment 7 Eric Carlson 2020-01-16 07:03:09 PST
Comment on attachment 387910 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:283
> +    CFArrayRef attachmentsArray = CMSampleBufferGetSampleAttachmentsArray(sample.platformSample().sample.cmSampleBuffer, true);
> +    for (CFIndex i = 0; i < CFArrayGetCount(attachmentsArray); ++i) {
> +        CFMutableDictionaryRef attachments = checked_cf_cast<CFMutableDictionaryRef>(CFArrayGetValueAtIndex(attachmentsArray, i));
> +        CFDictionarySetValue(attachments, kCMSampleAttachmentKey_DisplayImmediately, kCFBooleanTrue);
> +    }

You mentioned this hack in the ChangeLog, but it should have a FIXME here because we're going to need a remote display layer for MSE and it will need to honor timestamps.
Comment 8 youenn fablet 2020-01-16 07:12:23 PST
(In reply to Eric Carlson from comment #7)
> Comment on attachment 387910 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387910&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:283
> > +    CFArrayRef attachmentsArray = CMSampleBufferGetSampleAttachmentsArray(sample.platformSample().sample.cmSampleBuffer, true);
> > +    for (CFIndex i = 0; i < CFArrayGetCount(attachmentsArray); ++i) {
> > +        CFMutableDictionaryRef attachments = checked_cf_cast<CFMutableDictionaryRef>(CFArrayGetValueAtIndex(attachmentsArray, i));
> > +        CFDictionarySetValue(attachments, kCMSampleAttachmentKey_DisplayImmediately, kCFBooleanTrue);
> > +    }
> 
> You mentioned this hack in the ChangeLog, but it should have a FIXME here
> because we're going to need a remote display layer for MSE and it will need
> to honor timestamps.

OK, let's move it out of LocalSampleBufferDisplayLayer.mm for easier reuse.

I don't think this will apply to MSE though since presentation timing is key there.
MSE will probably not use LocalSampleBufferDisplayLayer.
In WebRTC, we are missing this information and are relying on attachment instead.
Comment 9 youenn fablet 2020-01-16 08:15:48 PST
Created attachment 387921 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-16 10:08:26 PST
The commit-queue encountered the following flaky tests while processing attachment 387921 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2020-01-16 10:09:15 PST
Comment on attachment 387921 [details]
Patch for landing

Clearing flags on attachment: 387921

Committed r254688: <https://trac.webkit.org/changeset/254688>
Comment 12 WebKit Commit Bot 2020-01-16 10:09:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2020-01-16 10:10:16 PST
<rdar://problem/58647949>
Comment 14 Matt Lewis 2020-01-17 16:40:23 PST
Reverted r254688 for reason:

This caused internal test crashes.

Committed r254778: <https://trac.webkit.org/changeset/254778>
Comment 15 youenn fablet 2020-01-20 00:50:49 PST
Created attachment 388216 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2020-01-20 02:38:13 PST
The commit-queue encountered the following flaky tests while processing attachment 388216 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2020-01-20 02:39:05 PST
Comment on attachment 388216 [details]
Patch for landing

Clearing flags on attachment: 388216

Committed r254817: <https://trac.webkit.org/changeset/254817>
Comment 18 WebKit Commit Bot 2020-01-20 02:39:07 PST
All reviewed patches have been landed.  Closing bug.