Closed Bug 1256626 Opened 4 years ago Closed 4 years ago

[e10s] Benchmark result isn't stored when e10s is enabled

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files)

Bug 1230265 added benchmarking VP9 in order to enable some features.

This do not work with e10s enabled.

Reason being Preferences::SetUint do not work when called from the content process

  // static
  nsresult
  Preferences::SetInt(const char* aPref, int32_t aValue)
  {
    ENSURE_MAIN_PROCESS("Cannot SetInt from content process:", aPref);
    NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
    return PREF_SetIntPref(aPref, aValue, false);
  }

Need to find another way to store the results.
tracking-e10s: --- → +
Priority: -- → P3
This will be used to save video benchmark results. For now only VP9 is handled.

Review commit: https://reviewboard.mozilla.org/r/40713/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40713/
Attachment #8731561 - Flags: review?(jmathies)
I don't see ContentParent.cpp aging very well if it's the unique mechanism/place to add a new service. 

But seeing how small the new NotifyBenchmarkResult code is, I don't see a more elegant way to do that.

I thought of simply calling a static method in VP9Benchmark from ContentParent, but it's no better really.
this is blocking us from enabling webm/VP9 (as used by YouTube) for all users with machines fast enough
Priority: P3 → P2
Attachment #8731561 - Flags: review?(jmathies) → review+
Comment on attachment 8731561 [details]
MozReview Request: Bug 1256626: P1. Add NotifyBenchmarkResult ipc methods. r?jimm

https://reviewboard.mozilla.org/r/40713/#review37683
Comment on attachment 8731562 [details]
MozReview Request: Bug 1256626: P2. Use NotifyBenchmarkResult to save VP9 result. r?jimm

https://reviewboard.mozilla.org/r/40715/#review37685
Attachment #8731562 - Flags: review?(jmathies) → review+
Thanks for the reviews
Comment on attachment 8731561 [details]
MozReview Request: Bug 1256626: P1. Add NotifyBenchmarkResult ipc methods. r?jimm

Patch request is for all changeset

Approval Request Comment
[Feature/regressing bug #]: 1256626
[User impact if declined]: We aim to enable VP9 to all machines fast enough to support it. It would also reduce issues seen with h264 hardware decoder.
[Describe test coverage new/current, TreeHerder]: in central for a few weeks.
[Risks and why]: Low, people may complain of a cpu usage spike as youtube prefers Vp9 over h264 if available. Overall playback quality should be greater however
[String/UUID change made/needed]: none
Attachment #8731561 - Flags: approval-mozilla-aurora?
Hi Jean-Yves, Chris, I was under the impression that VP9 would not be enabled in Fx47. If that is still the case, do we still need to uplift this to Aurora 47? Please let me know.
Flags: needinfo?(jyavenard)
Flags: needinfo?(cpeterson)
I only saw Chris marking 46as won't fix. 

We do want to enable vp9 in 47.
Flags: needinfo?(jyavenard)
e10s is not enabled in Beta 47, so we don't need to uplift this e10s fix to 47.
Flags: needinfo?(cpeterson)
Comment on attachment 8731561 [details]
MozReview Request: Bug 1256626: P1. Add NotifyBenchmarkResult ipc methods. r?jimm

Media team wants to enable VP9 on faster machines in 47, I have been told this is mostly a staged rollout and quality is acceptable. Aurora47+
Attachment #8731561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8731562 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.