Bug 199505

Summary: [GStreamer] media/video-volume.html broken after switching from cubic to linear scaling
Product: WebKit Reporter: Charlie Turner <cturner>
Component: WebKitGTKAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, commit-queue, jer.noble, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199559
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch none

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 1 Charlie Turner 2019-07-05 00:35:54 PDT
Created attachment 373482 [details]
Patch
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.