Bug 220972

Summary: Color Gamut for VP8 and VP9 seems wrong
Product: WebKit Reporter: Alex. Gouaillard <agouaillard>
Component: WebRTCAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hta, jean-yves.avenard, jer.noble, philipj, sergio, tommyw, tsavell, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=224960
https://bugs.webkit.org/show_bug.cgi?id=225257
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alex. Gouaillard 2021-01-26 05:05:47 PST
Problem:
Safari and iOS incorrectly interprets video levels with VP8/9 encoded content.

My understanding is that most UVC devices as well as Chrome and OBS publish methods by default use “partial” video levels. (Partial is OBS terminology, This is similar to the “legal” vs “Full Range" broadcast video levels). Almost all UVC/Webcams that I’ve used transmit with partial video levels by default. This is also true of hardware encoders. Chrome and Firefox both assume and interpret partial video levels, thus displaying a correct looking image. Safari and iOS for some reason expects and interprets video as Full Range levels. This means that blacks appear dark grey instead of truly black, and whites appear light grey instead of truly white. This has a significantly negative effect on the image accuracy and quality.

This is not a problem with H.264. Safari/iOS correctly interprets video as “partial” and looks correct and similar to Chrome.

I've found this issue is present in Millicast, Google Meet, and Jitsi Meet. It appears to be an issue specifically with how Safari/iOS interprets video that’s encoded in VP8/VP9.

Here is a link to a test video clip with a greyscale ramp. Also in this folder are two screenshots from a waveform monitor showing the difference between Chrome output and Safari output
https://f.io/qdncjeuk

Let me know if you have any questions.
Comment 1 Radar WebKit Bug Importer 2021-01-26 08:02:10 PST
<rdar://problem/73616455>
Comment 2 Jean-Yves Avenard [:jya] 2021-04-22 08:09:20 PDT
Created attachment 426812 [details]
Patch
Comment 3 youenn fablet 2021-04-22 08:21:54 PDT
Comment on attachment 426812 [details]
Patch

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

> LayoutTests/webrtc/video-vp8-videorange.html:43
> +        testRunner.setUserMediaPermission(true);

Not needed.

> LayoutTests/webrtc/video-vp8-videorange.html:45
> +    return new Promise((resolve, reject) => {

You could do: promise_test(async (test) => {
And then use await:
const remoteStream = await new Promise(...)

> LayoutTests/webrtc/video-vp8-videorange.html:81
> +}, "Ensuring connection state is connected");

Not sure this is actually needed, video.play() will resolve once it gets a remove video frame which means connection is ok.

> LayoutTests/webrtc/video-vp8-videorange.html:84
> +    return checkVideoBlack(false, canvas2, video).then(() => {

async (test) and await checkVideoBlack(...); would make it a bit nicer.

> LayoutTests/webrtc/video-vp8-videorange.html:88
> +        assert_equals([ pixelData[0], pixelData[1], pixelData[2] ].join(), [0, 0, 0].join());

Are we sure they will always be 0,0,0 given we do compression?

> LayoutTests/webrtc/video-vp8-videorange.html:91
> +        assert_equals([ pixelData[0], pixelData[1], pixelData[2] ].join(), [255, 255, 255].join());

Ditto here.
Comment 4 Jean-Yves Avenard [:jya] 2021-04-22 08:55:35 PDT
Comment on attachment 426812 [details]
Patch

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

>> LayoutTests/webrtc/video-vp8-videorange.html:45
>> +    return new Promise((resolve, reject) => {
> 
> You could do: promise_test(async (test) => {
> And then use await:
> const remoteStream = await new Promise(...)

Always amazed on how much code using JS promises can get simplified.

>> LayoutTests/webrtc/video-vp8-videorange.html:81
>> +}, "Ensuring connection state is connected");
> 
> Not sure this is actually needed, video.play() will resolve once it gets a remove video frame which means connection is ok.

I copied an existing test (video-mute-vp8.html) likely can be removed there too then.

>> LayoutTests/webrtc/video-vp8-videorange.html:88
>> +        assert_equals([ pixelData[0], pixelData[1], pixelData[2] ].join(), [0, 0, 0].join());
> 
> Are we sure they will always be 0,0,0 given we do compression?

I can't imagine sur simple pattern, with colours at the edge of the range to ever show colour artefacts.
Comment 5 Jean-Yves Avenard [:jya] 2021-04-22 09:02:52 PDT
Created attachment 426816 [details]
Patch
Comment 6 Jean-Yves Avenard [:jya] 2021-04-22 09:07:19 PDT
Created attachment 426817 [details]
Patch

Set reviewer name in Changelog
Comment 7 Eric Carlson 2021-04-22 09:25:57 PDT
Comment on attachment 426817 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        In memory of Dr. Alex. May he rest in peace.

❤️

> LayoutTests/webrtc/video-vp8-videorange.html:38
> +// Repeatidly redraw the checker to get the video stream going.

s/Repeatidly/Repeatedly/
Comment 8 Jean-Yves Avenard [:jya] 2021-04-22 17:24:02 PDT
Created attachment 426874 [details]
Patch

Fix typo, add link to bug 224960
Comment 9 EWS 2021-04-22 17:57:05 PDT
Committed r276478 (236937@main): <https://commits.webkit.org/236937@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426874 [details].
Comment 10 Truitt Savell 2021-04-30 16:13:49 PDT
It looks like the new test webrtc/video-vp8-videorange.html

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

is a constant failure on macmini8,1

History:
https://results.webkit.org/?suite=layout-tests&test=webrtc%2Fvideo-vp8-videorange.html

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