|Summary:||[GStreamer] media/video-volume.html broken after switching from cubic to linear scaling|
|Product:||WebKit||Reporter:||Charlie Turner <cturner>|
|Component:||WebKitGTK||Assignee:||Charlie Turner <cturner>|
|Severity:||Normal||CC:||bugs-noreply, calvaris, commit-queue, jer.noble, pnormand|
|Version:||WebKit Nightly Build|
Description Charlie Turner 2019-07-04 15:18:16 PDT
Reported test failure was, --- /home/phil/WebKit/WebKitBuild/Debug/layout-test-results/media/video-volume-expected.txt +++ /home/phil/WebKit/WebKitBuild/Debug/layout-test-results/media/video-volume-actual.txt @@ -12,7 +12,7 @@ EVENT(canplaythrough) EXPECTED (video.volume == '0') OK RUN(video.volume = 0.5) -EXPECTED (video.volume == '0.5') OK +EXPECTED (video.volume == '0.5'), OBSERVED '0.5000011920928955' FAIL TEST(video.volume = 1.5) THROWS(DOMException.INDEX_SIZE_ERR) OK TEST(video.volume = -0.5) THROWS(DOMException.INDEX_SIZE_ERR) OK END OF TEST This failure was caused by r246730, although it's not a regression. I believe it's a bug in the PA project. PA does it's own linear<->cubic conversions, which was part of the motivation for r246730 in the first place. However, despite the APIs for this conversion promising a inverse operation, we get, pa_sw_volume_to_linear(pa_sw_volume_from_linear(0.5)) -> 0.500001 The reason this worked before was due to the cubic scaling we initially applied. When you cube 0.5, you get 0.125 and pa_sw_volume_to_linear(pa_sw_volume_from_linear(0.125)) -> 0.125, so the cbrt gives back 0.5. The test passes, but not in the way you would expect. I believe the bug in PA is due to their use of implicitly widening 32-bit integers to 64-bits in their interpolation code, and getting garbage (undefined memory) in the resulting value which when cast back to float, gives us a rounding issue. I am talking with the developers about it.
Comment 2 Charlie Turner 2019-07-05 00:45:07 PDT
The PA devs do not agree its a bug, so we have to check within a tolerance
Comment 3 Charlie Turner 2019-07-05 02:40:52 PDT
CC Jer, do you have any thoughts about this? Was advised it may not be considered an acceptable solution, although platform overrides would probably be equally flakey across toolchain versions / architectures. If you don't like it, I will push harder on the PA side. For reference, the upstream bug is reported here: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/697
Comment 4 Xabier Rodríguez Calvar 2019-07-05 03:04:37 PDT
Comment on attachment 373482 [details] Patch Even when I r+ this, let's wait for Jer to have the final word.
Comment 5 Jer Noble 2019-07-05 07:52:39 PDT
Comment on attachment 373482 [details] Patch I’m fine with this to fix the test, but I think we should spin off a new bug to investigate how to fix this for good (in such a way that “volume = foo; assert(volume == foo)” always succeeds.
Comment 6 Charlie Turner 2019-07-07 01:59:18 PDT
Created attachment 373593 [details] Patch for landing
Comment 7 Philippe Normand 2019-07-07 14:17:03 PDT
Comment on attachment 373593 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=373593&action=review This patch looks like a mix of 2 different patches. Pulling out of cq. > LayoutTests/media/video-seek-to-current-time.html:49 > + consoleWrite("runTest()"); Woot?
Comment 8 Charlie Turner 2019-07-08 03:24:50 PDT
Created attachment 373620 [details] Patch I blame webkit-upload :-P, I have two patches in my local working copy and they both got uploaded. Let me cleanup and resubmit.
Comment 9 WebKit Commit Bot 2019-07-08 04:47:19 PDT
Comment on attachment 373620 [details] Patch Clearing flags on attachment: 373620 Committed r247207: <https://trac.webkit.org/changeset/247207>
Comment 10 WebKit Commit Bot 2019-07-08 04:47:21 PDT
All reviewed patches have been landed. Closing bug.