Bug 205922

Summary:

RemoteVideoSample should be able to support canvas video samples

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebRTC

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, tsavell, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Description youenn fablet 2020-01-08 05:56:47 PST

RemoteVideoSample should be able to support canvas video samples

Comment 1 youenn fablet 2020-01-08 06:03:30 PST

Created attachment 387100 [details]
Patch

Comment 2 Eric Carlson 2020-01-08 09:21:27 PST

Comment on attachment 387100 [details]
Patch

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

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:56
> +    auto ioSurface =  IOSurface::create(size, sRGBColorSpaceRef(), IOSurface::Format::RGBA);

Minor nit: you might as well move this down a few lines in case CVPixelBufferLockBaseAddress fails.

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:109
> +            RELEASE_LOG_ERROR(Media, "RemoteVideoSample::create: CVPixelBufferGetIOSurface returned nullptr");

This log message is now incorrect.

> LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-gpuprocess-expected.txt:5
> +Harness Error (TIMEOUT), message = null
> +
> +TIMEOUT Verify MediaRecorder is working in GPUProcess Test timed out

Maybe add a comment to the ChangeLog about why the new test fails?

Comment 3 youenn fablet 2020-01-09 01:37:04 PST

(In reply to Eric Carlson from comment #2)
> Comment on attachment 387100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387100&action=review
> 
> > Source/WebCore/platform/graphics/RemoteVideoSample.cpp:56
> > +    auto ioSurface =  IOSurface::create(size, sRGBColorSpaceRef(), IOSurface::Format::RGBA);
> 
> Minor nit: you might as well move this down a few lines in case
> CVPixelBufferLockBaseAddress fails.

OK

> > Source/WebCore/platform/graphics/RemoteVideoSample.cpp:109
> > +            RELEASE_LOG_ERROR(Media, "RemoteVideoSample::create: CVPixelBufferGetIOSurface returned nullptr");
> 
> This log message is now incorrect.

Updated.

> > LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-gpuprocess-expected.txt:5
> > +Harness Error (TIMEOUT), message = null
> > +
> > +TIMEOUT Verify MediaRecorder is working in GPUProcess Test timed out
> 
> Maybe add a comment to the ChangeLog about why the new test fails?

Last minute change to the test made it fail, it is missing a t.done.
Will fix it.

Comment 4 youenn fablet 2020-01-09 01:37:46 PST

Created attachment 387200 [details]
Patch for landing

Comment 5 WebKit Commit Bot 2020-01-09 06:31:16 PST

Comment on attachment 387200 [details]
Patch for landing

Clearing flags on attachment: 387200

Committed r254256: <https://trac.webkit.org/changeset/254256>

Comment 6 WebKit Commit Bot 2020-01-09 06:31:17 PST

All reviewed patches have been landed.  Closing bug.

Comment 7 Radar WebKit Bug Importer 2020-01-09 06:32:13 PST

<rdar://problem/58442026>

Comment 8 Truitt Savell 2020-01-17 13:15:46 PST

The new test http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-gpuprocess.html 

added in https://trac.webkit.org/changeset/254256/webkit

is a flaky failure on Mojave wk2

Tracking in https://bugs.webkit.org/show_bug.cgi?id=206437