Summary: | Color Gamut for VP8 and VP9 seems wrong | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex. Gouaillard <agouaillard> | ||||||||||
Component: | WebRTC | Assignee: | 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
Alex. Gouaillard
2021-01-26 05:05:47 PST
Created attachment 426812 [details]
Patch
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 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. Created attachment 426816 [details]
Patch
Created attachment 426817 [details]
Patch
Set reviewer name in Changelog
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/ Created attachment 426874 [details] Patch Fix typo, add link to bug 224960 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]. 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 |