Bug 199505 – [GStreamer] media/video-volume.html broken after switching from cubic to linear scaling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

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.

[Charlie Turner]

Created attachment 373482 [details]
Patch

[Charlie Turner]

The PA devs do not agree its a bug, so we have to check within a tolerance

[Charlie Turner]

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

[Xabier Rodríguez Calvar]

Comment on attachment 373482 [details]
Patch

Even when I r+ this, let's wait for Jer to have the final word.

[Jer Noble]

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.

[Charlie Turner]

Created attachment 373593 [details]
Patch for landing

[Philippe Normand]

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?

[Charlie Turner]

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.

[WebKit Commit Bot]

Comment on attachment 373620 [details]
Patch

Clearing flags on attachment: 373620

Committed r247207: <https://trac.webkit.org/changeset/247207>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.