Uplift Bug 1299105 Bug 1320618 into Aurora(52) and Beta(51)

RESOLVED INVALID

Status

()

RESOLVED INVALID
2 years ago
a year ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

2 years ago
This bug is for uplifting Bug 1299105 Bug 1320618.

I will provide a merged patch for Aurora and Beta.

And asking for approval.

Comment 1

2 years ago
The procedure generally is to attach a patch to the bug itself, setting flags "approval‑mozilla‑aurora" and/or "approval‑mozilla‑beta", and filling out the uplift request template in the attachment comment (see https://wiki.mozilla.org/Release_Management/Uplift_rules for guidelines).
(Assignee)

Comment 2

2 years ago
OK, event  dealing with several dependency bugs?

Comment 3

2 years ago
(In reply to James Cheng[:JamesCheng] from comment #2)
> OK, event  dealing with several dependency bugs?

I believe so. I usually explicitly state in the uplift request comment something like "this bug must be uplited with this other bug, because so and so". Release drivers will keep that in mind when making a decision.
(Assignee)

Comment 4

2 years ago
Thank you, I will do that separately.
Reiterating what I wrote in the other bug. 

It sounds very dangerous to me to want to uplift this immediately when it has received no testing coverage whatsoever and we don't know if all devices support it.

This has the potential to break all media playback for a problem that isn't that serious to start with (which is that we have a slight pause when we change resolution which typically occurs once per playback)
It doesn't fix a crash, nor a security issue. It doesn't even fix a missing functionality.
(Assignee)

Comment 6

2 years ago
Hi Jya,

I agree with your concern and do you think I should pref on the "media.decoder.recycle.enabled" only on mobile.js for Fennec to observe if it is any side effect happened on nightly end-user?

If so, I will open a bug to turn on the preference.

Thank you very much.
Flags: needinfo?(jyavenard)
The code is only doing something on fennec. It's on fennec the pref should be off by default. 
At worse have it on, on nightly only. 

IMHO this isn't something that should be uplifted in a hurry. It should ride the train. And if it was me I wouldn't enable that feature at all. I've already expressed my concern yet you continue on a different path.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> The code is only doing something on fennec. It's on fennec the pref should
> be off by default. 
> At worse have it on, on nightly only. 
> 
> IMHO this isn't something that should be uplifted in a hurry. It should ride
> the train. And if it was me I wouldn't enable that feature at all. I've
> already expressed my concern yet you continue on a different path.
After discussing with John and Kilik, we believe James solution is better. :-)
From my understanding, bug 1299068 is to fix the issue that the SurfaceTexture is always updated to the latest decoded frame. That is a design problem in current Android PDM. John is going to fix it. After that bug is fixed, we still need to have James' solution to make playback smooth since recreating decoder takes time and increasing video buffer queue cannot fix this problem and may cause some side effects, like decoder may not have enough empty buffers for decoded frames. Those decoded buffers are shared with decoder and compositor. We cannot increase that number since it is decided in Android codes. And if we hold too many of them in gecko, decoder may fail to decode frames. That's what I understand. I could be wrong. John should be the right person to explain more. If needed, we can discuss it next Monday or Tue.
And I also agree we should monitor it on nightly first. (I thought it is already on)
(In reply to Blake Wu [:bwu][:blakewu] from comment #8)
> (In reply to Jean-Yves Avenard [:jya] from comment #7)
> > The code is only doing something on fennec. It's on fennec the pref should
> > be off by default. 
> > At worse have it on, on nightly only. 
> > 
> > IMHO this isn't something that should be uplifted in a hurry. It should ride
> > the train. And if it was me I wouldn't enable that feature at all. I've
> > already expressed my concern yet you continue on a different path.
> After discussing with John and Kilik, we believe James solution is better.
> :-)
> From my understanding, bug 1299068 is to fix the issue that the
> SurfaceTexture is always updated to the latest decoded frame. That is a
> design problem in current Android PDM. John is going to fix it. After that
> bug is fixed, we still need to have James' solution to make playback smooth
> since recreating decoder takes time and increasing video buffer queue cannot
> fix this problem and may cause some side effects, like decoder may not have
> enough empty buffers for decoded frames. Those decoded buffers are shared
> with decoder and compositor. We cannot increase that number since it is
> decided in Android codes. And if we hold too many of them in gecko, decoder
> may fail to decode frames. That's what I understand. I could be wrong. John
> should be the right person to explain more. If needed, we can discuss it
> next Monday or Tue.

 I wouldn't say that's a better solution, but it's the only one we can come up with so far.
 And I agree with jya that this should not be uplifted in a hurry. Please wait until you guys meet and walk him through the concerns we discussed offline yesterday. He may think of some really better solution, like some clever way to hide the recyclability thing in the PDM/data decoder level.
Since the problem bug 1299105 is very bad. Let's see if we can first uplift to Aurora or not. I am asking QA to have more tests.
This bug is for old releases and no longer relevant.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.