Opened 16 months ago

Closed 8 months ago

Last modified 8 months ago

#22548 closed defect (fixed)

Firefox downgrades VP9 videos to VP8 when measured performance is not enough

Reported by: cypherpunks Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting, TorBrowserTeam201801R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On systems where H.264 is not available or no HWA, VP9 is preferred. But in Tor Browser 7.0 all youtube videos degraded to VP8.

Child Tickets

Attachments (2)

Change History (24)

comment:1 Changed 16 months ago by gk

Status: newneeds_information

Interesting. Does that happen with a normal Firefox 52 ESR as well? If not, how can I reproduce that degradation (like seeing that it is indeed VP8 that gets used and not VP9 etc.)?

Last edited 16 months ago by gk (previous) (diff)

comment:2 Changed 16 months ago by cypherpunks

Resolution: invalid
Status: needs_informationclosed
Last edited 15 months ago by cypherpunks (previous) (diff)

comment:3 Changed 15 months ago by cypherpunks

Resolution: invalid
Status: closedreopened

No, it's a Firefox 52 ESR "feature".

comment:4 Changed 15 months ago by gk

Resolution: wontfix
Status: reopenedclosed

If you feel this is a bug please open a ticket at Mozilla's bug tracker or link to one. If that is indeed a "feature", then we won't fix that just for Tor Browser.

comment:5 Changed 15 months ago by cypherpunks

Keywords: tbb-fingerprinting added
Resolution: wontfix
Status: closedreopened
Summary: Youtube downgrades VP9 videos to VP8 when can't find H.264Firefox downgrades VP9 videos to VP8 when measured performance is not enough

Here it is https://bugzilla.mozilla.org/show_bug.cgi?id=1230265
And this is a fingerprinting vector for Tor Browser.

comment:6 Changed 15 months ago by cypherpunks

The reality is https://bugzilla.mozilla.org/show_bug.cgi?id=1198715#c17
And "media.mediasource.webm.enabled is false by default on all platforms but linux", because of that bug is WONTFIX.

Last edited 15 months ago by cypherpunks (previous) (diff)

comment:7 Changed 15 months ago by cypherpunks

Keywords: tbb-easy added

"We've found that some videos *only* play if you have VP9 enabled, which is why we enabled it unconditionally." from https://bugzilla.mozilla.org/show_bug.cgi?id=1266102, which disables "feature" on Android.

comment:8 Changed 15 months ago by gk

Thanks for digging out the Mozilla bugs. Actually, this ticket is not tbb-easy as it is not even clear how we should solve this problem.

comment:9 in reply to:  8 ; Changed 15 months ago by cypherpunks

Replying to gk:

Thanks for digging out the Mozilla bugs. Actually, this ticket is not tbb-easy as it is not even clear how we should solve this problem.

The same way as on Android. See patch from that ticket, we need IsVP9DecodeFast() be always true for all platforms, e.g. by

-#ifdef MOZ_WIDGET_ANDROID
+#ifdef 1

and also

-#ifndef MOZ_WIDGET_ANDROID
-#include "WebMSample.h"
-#endif

comment:10 in reply to:  9 Changed 15 months ago by gk

Replying to cypherpunks:

Replying to gk:

Thanks for digging out the Mozilla bugs. Actually, this ticket is not tbb-easy as it is not even clear how we should solve this problem.

The same way as on Android. See patch from that ticket, we need IsVP9DecodeFast() be always true for all platforms, e.g. by

-#ifdef MOZ_WIDGET_ANDROID
+#ifdef 1

and also

-#ifndef MOZ_WIDGET_ANDROID
-#include "WebMSample.h"
-#endif

That's one option. However, note https://bugzilla.mozilla.org/show_bug.cgi?id=1286738 where this solution got basically reverted. (and we need to take mobile more and more into account as we plan to provide a Tor Browser on Mobile)

comment:11 Changed 15 months ago by gk

Oh, and there might already have been a fingerprinting vector before this change got implemented, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1230265#c43

We're increasing the number of users for whom we're enabling VP9 from about 10% (people who don't have H.264) to 50% (people with fast enough machines).

comment:12 Changed 15 months ago by cypherpunks

Well, then + return true;, of course. Unfortunately, setting media.mediasource.webm.enabled to true doesn't work. Dances with HWA detection are for Mozilla only, Tor Browser has all HW decoders disabled. Therefore, enabling VP9 unconditionally is the only way for it (this affects mobile, but reduces fingerprint). This also fixes the issue from your second comment.

comment:13 Changed 12 months ago by cypherpunks

Status: reopenedneeds_review

Set media.benchmark.vp9.threshold to 0 to fix this ticket without patching.

comment:14 in reply to:  13 ; Changed 12 months ago by gk

Keywords: tbb-easy removed
Status: needs_reviewneeds_information

Replying to cypherpunks:

Set media.benchmark.vp9.threshold to 0 to fix this ticket without patching.

Thanks for this suggestion. What are the implications of this proposed fix? (We'd need a patch for it as well)

comment:15 in reply to:  14 Changed 12 months ago by cypherpunks

Status: needs_informationnew

Replying to gk:

Replying to cypherpunks:

Set media.benchmark.vp9.threshold to 0 to fix this ticket without patching.

Thanks for this suggestion.

This is not only a suggestion, but the result of code analysis.

What are the implications of this proposed fix? (We'd need a patch for it as well)

All clients will get better experience and lower traffic, because TBB doesn't use Use hardware acceleration when available anyway. (Code analysis shows that you don't need a patch, -#include "WebMSample.h" is for cleanup only.)

comment:16 Changed 9 months ago by ffmancera

Status: newneeds_review

Here is the patch, I hope everything is fine!

comment:17 Changed 9 months ago by cypherpunks

Keywords: TorBrowserTeam201801R added

Let's see how it goes.

comment:18 Changed 9 months ago by tom

A few notes. I reviewed mozilla-central and -esr52.

0: I agree, this is a hardware fingerprinting vector (albeit a small one, 1 bit of info). We should set the pref either very high (disabling VP9 for everyone) or to 0 (enabling it for everyone). Telemetry indicates more users fall below the 150 fps choice than above: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-12-26&keys=__none__!__none__!__none__&max_channel_version=release%252F57&measure=VIDEO_VP9_BENCHMARK_FPS&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-11-12&table=0&trim=1&use_submission_date=0

1: The correct place to put prefs is browser/app/profile/000-tor-browser.js It's best to keep the prefs all in one place. (Also see #4)

2: Setting the pref does not enable vp9 on Android. That's probably fine, since all Androids will be hardcoded to false (no fingerprinting) and the point of disabling vp9 there is because it's not fast enough for phones. (True for -central and esr)

3: Setting the pref doesn't enable it on Macs. Same problem, except Macs are hardcoded false because it spins up the fan and annoys people. (Only true for central) Maybe this #ifdef should be backported to esr52 for TB? If it is I think it should be a separate commit/patch to make rebasing easier (on rebase we can just drop the individual patch entirely.)

4: The benchmark is skipped correctly when the pref is set manually. However, setting the pref in all.js means the pref won't have a user value, and the benchmark will be run. That's bad, so that's two reasons it should be in 000-tor-browser.js

5: I assume, but never verified, that prefs set in 000-tor-browser.js are considered to 'have a user value'.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1428351 for upstream.

Thanks for this work, I think once 1 is fixed, we should apply it. gk - do you want to backport the Mac hardcode in 3?

comment:19 Changed 8 months ago by ffmancera

Status: needs_reviewnew

I think I solved "1". Hope everything is fine now :)

comment:20 Changed 8 months ago by ffmancera

Status: newneeds_review

comment:21 in reply to:  18 Changed 8 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to tom:

Thanks for this work, I think once 1 is fixed, we should apply it. gk - do you want to backport the Mac hardcode in 3?

Seems reasonable. We are back then to platform discrimination where we had possible user discrimination per platform before, so I think that's a win. I backported the fix to https://bugzilla.mozilla.org/show_bug.cgi?id=1403412 as well (assuming that was actually meant :) ).

Both is available on tor-browser-52.5.2esr-8.0-1 (commits 9080c51a22a03aaf8e49d5552253ed716414c760 and deaabb54809b2de56b2b5ce98a0f629c1455fcf5) and the plan is to have it in the next alpha.

comment:22 Changed 8 months ago by gk

So, the patch in Benchmark.cpp does not even compile actually. Here is the output with GCC 5.4.0:

/var/tmp/build/firefox-f0b673dbac3a/dom/media/Benchmark.cpp: In static member function 'static bool mozilla::VP9Benchmark::IsVP9DecodeFast()':
/var/tmp/build/firefox-f0b673dbac3a/dom/media/Benchmark.cpp:83:60: error: call of overloaded 'GetUint(const char [30], int)' is ambiguous
     Preferences::GetUint("media.benchmark.vp9.threshold", 0);
                                                            ^
In file included from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/xpcpublic.h:28:0,
                 from /var/tmp/build/firefox-f0b673dbac3a/dom/base/nsJSEnvironment.h:19,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CallbackObject.h:31,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:20,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ToJSValue.h:12,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/IterableIterator.h:34,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/MediaKeyStatusMapBinding.h:11,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/CDMCaps.h:18,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/CDMProxy.h:10,
                 from /var/tmp/build/firefox-f0b673dbac3a/dom/media/MediaDecoder.h:11,
                 from /var/tmp/build/firefox-f0b673dbac3a/dom/media/ADTSDecoder.h:10,
                 from /var/tmp/build/firefox-f0b673dbac3a/dom/media/ADTSDecoder.cpp:7,
                 from /var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dom/media/Unified_cpp_dom_media0.cpp:2:
/var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Preferences.h:122:19: note: candidate: static uint32_t mozilla::Preferences::GetUint(const char*, uint32_t)
   static uint32_t GetUint(const char* aPref, uint32_t aDefault = 0)
                   ^
/var/tmp/build/firefox-f0b673dbac3a/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Preferences.h:173:19: note: candidate: static nsresult mozilla::Preferences::GetUint(const char*, uint32_t*)
   static nsresult GetUint(const char* aPref, uint32_t* aResult)
                   ^

Now, if you look closer at the function in questions you'll see:

uint32_t hadRecentUpdate = Preferences::GetUint(sBenchmarkFpsVersionCheck, 0U)

And, yes, replacing 0 with 0U in the patch I merged fixes the compilation. I've pushed a fixup commit to tor-browser-52.6.0esr-8.0-1 (commit d93d0469d2b21b67869369063e17074563a49abf).

Note: See TracTickets for help on using tickets.