Bug 199505 - [GStreamer] media/video-volume.html broken after switching from cubic to linear scaling
Summary: [GStreamer] media/video-volume.html broken after switching from cubic to line...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
Depends on:
Reported: 2019-07-04 15:18 PDT by Charlie Turner
Modified: 2019-07-08 04:47 PDT (History)
5 users (show)

See Also:

Patch (3.75 KB, patch)
2019-07-05 00:35 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (15.75 KB, patch)
2019-07-07 01:59 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (3.75 KB, patch)
2019-07-08 03:24 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 @@
 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

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]
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]

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]

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()");

Comment 8 Charlie Turner 2019-07-08 03:24:50 PDT
Created attachment 373620 [details]

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]

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.