1347439 - Uplift bug 1336792 and 1344649 into Beta(53)

Status: RESOLVED INVALID

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

Description

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

Cherry picking from:
- bug 1336792
- bug 1344649 to fix bug 1336431

Comment 1

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

Attachment: Bug 1347439 - part 1: extract DurationMap and make it thread safe.

Cherry-picked from bug 1336792 part 1.

Review commit: https://reviewboard.mozilla.org/r/120486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120486/

Comment 2

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

Attachment: Bug 1347439 - part 2: use pts to index input duration.

Cherry-picked from bug 1336792 part 2

Review commit: https://reviewboard.mozilla.org/r/120488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120488/

Comment 3

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

Attachment: Bug 1347439 - part 3: release outputs not sent for rendering.

Cherry-picked from bug 1336792 part 3

Review commit: https://reviewboard.mozilla.org/r/120490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120490/

Comment 4

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

Attachment: Bug 1347439 - part 4: use picture instead of display size to construct Image.

Cherry-picked from bug 1344649 part 1 for bug 1336431

Review commit: https://reviewboard.mozilla.org/r/120492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120492/

Comment 5

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

Attachment: Bug 1347439 - part 5: let VideoData::CreateFromImage() accept only neccessary parameters.

Cherry-picked from bug 1344649 part 2 for bug 1336431

Review commit: https://reviewboard.mozilla.org/r/120494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120494/

Comment 6

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

Attachment: Bug 1347439 - part 6: rename DurationMap and turn it into a generic class.

Cherry-picked from bug 1344649 part 3 for bug 1336431

Review commit: https://reviewboard.mozilla.org/r/120496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120496/

Comment 7

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

Attachment: Bug 1347439 - part 7: make sure TrackInfo in the sample is up to date.

Cherry-picked from bug 1344649 part 5 for bug 1336431

Review commit: https://reviewboard.mozilla.org/r/120498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120498/

Comment 8

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

Attachment: Bug 1347439 - part 8: store frame sizes in queue.

Cherry-picked from bug 1344649 part 4 for bug 1336431

Review commit: https://reviewboard.mozilla.org/r/120500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120500/

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847489 [details]
Bug 1347439 - part 1: extract DurationMap and make it thread safe.

https://reviewboard.mozilla.org/r/120486/#review122410

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847490 [details]
Bug 1347439 - part 2: use pts to index input duration.

https://reviewboard.mozilla.org/r/120488/#review122412

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847491 [details]
Bug 1347439 - part 3: release outputs not sent for rendering.

https://reviewboard.mozilla.org/r/120490/#review122414

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847493 [details]
Bug 1347439 - part 5: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/120494/#review122418

Comment 14

[Jean-Yves Avenard [:jya]]

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

Comment 15

[Jean-Yves Avenard [:jya]]

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

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847496 [details]
Bug 1347439 - part 8: store frame sizes in queue.

https://reviewboard.mozilla.org/r/120500/#review122426

Comment 17

[Jean-Yves Avenard [:jya]]

those are just uplifts, I'm not sure it needed reviews...

Comment 18

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

Thanks for the instant response. :)

Comment 19

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

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

Comment 20

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

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 21

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

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 22

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

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

Comment 23

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

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

Comment 24

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

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

Comment 25

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

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 26

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

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 27

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

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

Comment 28

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

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

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 31

[Jean-Yves Avenard [:jya]]

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.

Comment 32

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

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.

Comment 33

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

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

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 35

[Jean-Yves Avenard [:jya]]

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

Comment 36

[Mike Taylor [:miketaylr]]

Liz, does Comment #35 change anything here?

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 38

[Julien Cristau [:jcristau]]

Is there anything left to track here?  oop decoding was disabled for 53, and afaict bugs 1336792 and 1344649 are in 54 already?

Comment 39

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

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

Comment 40

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

Sorry for the wrong resolution. It should be INVALID.