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

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[James Cheng[:JamesCheng]]

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

[:Grisha Kruglov]

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).

Comment 2

[James Cheng[:JamesCheng]]

OK, event  dealing with several dependency bugs?

Comment 3

[:Grisha Kruglov]

(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.

Comment 4

[James Cheng[:JamesCheng]]

Thank you, I will do that separately.

Comment 5

[Jean-Yves Avenard [:jya]]

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.

Comment 6

[James Cheng[:JamesCheng]]

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.

Comment 7

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[Blake Wu [:bwu][:blakewu]]

(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.

Comment 9

[Blake Wu [:bwu][:blakewu]]

And I also agree we should monitor it on nightly first. (I thought it is already on)

Comment 10

[John Lin [:jhlin][:jolin]]

(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.

Comment 11

[Blake Wu [:bwu][:blakewu]]

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.

Comment 12

[John Lin [:jhlin][:jolin]]

This bug is for old releases and no longer relevant.