Closed Bug 1347439 Opened 5 years ago Closed 5 years ago

Uplift bug 1336792 and 1344649 into Beta(53)

Categories

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

53 Branch
defect
Not set
normal

Tracking

(firefox52 wontfix, firefox53 wontfix, firefox54 unaffected, firefox55 unaffected)

RESOLVED INVALID
Tracking Status
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- unaffected
firefox55 --- unaffected

People

(Reporter: jhlin, Assigned: jhlin)

Details

(Keywords: regression)

Attachments

(8 obsolete files)

Cherry picking from:
- bug 1336792
- bug 1344649 to fix bug 1336431
Comment on attachment 8847489 [details]
Bug 1347439 - part 1: extract DurationMap and make it thread safe.

https://reviewboard.mozilla.org/r/120486/#review122410
Attachment #8847489 - Flags: review?(jyavenard) → review+
Comment on attachment 8847490 [details]
Bug 1347439 - part 2: use pts to index input duration.

https://reviewboard.mozilla.org/r/120488/#review122412
Attachment #8847490 - Flags: review?(jyavenard) → review+
Comment on attachment 8847491 [details]
Bug 1347439 - part 3: release outputs not sent for rendering.

https://reviewboard.mozilla.org/r/120490/#review122414
Attachment #8847491 - Flags: review?(jyavenard) → review+
Comment on attachment 8847492 [details]
Bug 1347439 - part 4: use picture instead of display size to construct Image.

https://reviewboard.mozilla.org/r/120492/#review122416
Attachment #8847492 - Flags: review?(jyavenard) → review+
Comment on attachment 8847493 [details]
Bug 1347439 - part 5: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/120494/#review122418
Attachment #8847493 - Flags: review?(jyavenard) → review+
Comment on attachment 8847494 [details]
Bug 1347439 - part 6: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/120496/#review122420
Attachment #8847494 - Flags: review?(jyavenard) → review+
Comment on attachment 8847495 [details]
Bug 1347439 - part 7: make sure TrackInfo in the sample is up to date.

https://reviewboard.mozilla.org/r/120498/#review122422
Attachment #8847495 - Flags: review?(jyavenard) → review+
Comment on attachment 8847496 [details]
Bug 1347439 - part 8: store frame sizes in queue.

https://reviewboard.mozilla.org/r/120500/#review122426
Attachment #8847496 - Flags: review?(jyavenard) → review+
those are just uplifts, I'm not sure it needed reviews...
Thanks for the instant response. :)
Comment on attachment 8847489 [details]
Bug 1347439 - part 1: extract DurationMap and make it thread safe.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames shown at wrong time
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback time
[String changes made/needed]:none
Attachment #8847489 - Flags: approval-mozilla-beta?
Comment on attachment 8847490 [details]
Bug 1347439 - part 2: use pts to index input duration.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames shown at wrong time
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback time
[String changes made/needed]:none
Comment on attachment 8847490 [details]
Bug 1347439 - part 2: use pts to index input duration.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames shown at wrong time
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback time
[String changes made/needed]:none
Attachment #8847490 - Flags: approval-mozilla-beta?
Comment on attachment 8847491 [details]
Bug 1347439 - part 3: release outputs not sent for rendering.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames shown at wrong time
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback time
[String changes made/needed]:none
Attachment #8847491 - Flags: approval-mozilla-beta?
Comment on attachment 8847492 [details]
Bug 1347439 - part 4: use picture instead of display size to construct Image.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Attachment #8847492 - Flags: approval-mozilla-beta?
Comment on attachment 8847493 [details]
Bug 1347439 - part 5: let VideoData::CreateFromImage() accept only neccessary parameters.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Attachment #8847493 - Flags: approval-mozilla-beta?
Comment on attachment 8847494 [details]
Bug 1347439 - part 6: rename DurationMap and turn it into a generic class.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Comment on attachment 8847494 [details]
Bug 1347439 - part 6: rename DurationMap and turn it into a generic class.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Attachment #8847494 - Flags: approval-mozilla-beta?
Comment on attachment 8847495 [details]
Bug 1347439 - part 7: make sure TrackInfo in the sample is up to date.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Attachment #8847495 - Flags: approval-mozilla-beta?
Comment on attachment 8847496 [details]
Bug 1347439 - part 8: store frame sizes in queue.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video frames displayed in wrong size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it only changes the frame playback size
[String changes made/needed]:none
Attachment #8847496 - Flags: approval-mozilla-beta?
Assignee: nobody → jolin
What is the impact here on users? Who is affected by this? Can you explain why we need to uplift to 53 or 54?

I have just tried to figure this out from skimming through this, bug 1257777, bug 1336792, bug 1344649, bug 1336431, bug 1340124 and even a level deeper. Confusing. 

This looks like a big change to introduce into beta 53. 

It will help (maybe to get it into 54) if you can explain what this is for, who it's for, why we need it, and how QA can verify or test it.
Flags: needinfo?(jolin)
Marking this as some sort of regression, since it seems to be one from quickly looking over the list of related bugs. Not sure which branches it would affect.
Attachment #8847489 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847490 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847491 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847492 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847493 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847494 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847495 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8847496 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
In beta, the code is disabled by default so likely okay to not have it there. But see below

In aurora, it was enabled and the previous code was remove (apparently I went a tad hastily there). So it's important that it was uplifted to aurora. 

Back to beta. The code enabled by default also had similar problems: races, invalid aspect ratio displayed, invalid resolution. We may want to turn on the new code instead, in which case we definitely want the uplift.
In fact the remote decoding is enabled since 53, which is in beta now.

If not uplifted, user might experience one or both the following issues when playing specific video:
1. slight jankiness (when video has variable frame rate)
2. incorrect size/aspect ration (when video changes resolution)

The number of patches in this changeset might look scary, but this is not that risky as it seems. Only 3 real logic changes in 2 classes, the others are just refactoring or code cleaning up done in the process of developing patches to make actual changes simpler.
Flags: needinfo?(jolin)
(In reply to John Lin [:jolin][:jhlin] from comment #32)
> In fact the remote decoding is enabled since 53, which is in beta now.
To be clear, what John said about "remote decoding" here is listed as "Move Android audio/video decoder out of application process for better stability during media playback" in 53 release notes[1].
Maybe we should ask QA to test it. 

[1]https://www.mozilla.org/en-US/firefox/android/53.0beta/releasenotes/
Flags: needinfo?(lhenry)
If this isn't enabled by default in beta we probably should take out the release note.
If we don't intend to ship it in 53 I'd rather not uplift as we head into beta 5.
Flags: needinfo?(lhenry)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> If this isn't enabled by default in beta we probably should take out the
> release note.
> If we don't intend to ship it in 53 I'd rather not uplift as we head into
> beta 5.

my mistake.. it is enable in 53..

We should uplift these changes.

most of the changes is just to revert a change so that the change in nightly could be more easily done.

the final diff is rather small.
Liz, does Comment #35 change anything here?
Flags: needinfo?(lhenry)
I don't think this is a good idea for 53, we are just shipping the release candidate build today. We lived with it for 52, let's plan this better and do some testing for 54.
Flags: needinfo?(lhenry)
Is there anything left to track here?  oop decoding was disabled for 53, and afaict bugs 1336792 and 1344649 are in 54 already?
(In reply to Julien Cristau [:jcristau] from comment #38)
> Is there anything left to track here?  oop decoding was disabled for 53, and
> afaict bugs 1336792 and 1344649 are in 54 already?

 Thanks for reminding me. Uplifting is no longer needed for OOP decoding was disabled in bug 1350209.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jolin)
Resolution: --- → WONTFIX
Sorry for the wrong resolution. It should be INVALID.
Resolution: WONTFIX → INVALID
Attachment #8847489 - Attachment is obsolete: true
Attachment #8847490 - Attachment is obsolete: true
Attachment #8847491 - Attachment is obsolete: true
Attachment #8847492 - Attachment is obsolete: true
Attachment #8847493 - Attachment is obsolete: true
Attachment #8847494 - Attachment is obsolete: true
Attachment #8847495 - Attachment is obsolete: true
Attachment #8847496 - Attachment is obsolete: true
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.