1082555 - video clips and there title gets interchanged during suspend resume operation

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Pooja]

Attachment: steps to reproduce

STR:

1. Open video app
2. Play video
3. Go back to thumbnail and start another video
4. Go back to thumbnail and play video you played at step #2
5. Suspend device using Power button
6. Resume target using same power Button

Reproducible 3/5
Actual result:
 The video clip title remains same but video changed with some other existing clip in video app.

Expected Result:
  The video and its title should be same as before suspend

In attached video, 
1. Notice 1st and 3rd clip name at 0:00
2. Attached video is quite long so kindly watch 1:00 -1:16 - time frame when issue reproduced

Gaia and Gecko info:
Gaia "f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1"
Gecko "db7fce920e7d782d9f601384dc95924abcdaeeb8"

Comment 1

[Pooja]

[Blocking Requested - why for this release]:

Comment 2

[Hema Koka [:hema]]

Punam, please investigate and see if you can reproduce this on Flame. 

Blocking: Title and Video clip should match regardless of turning on/off power button. 

Thanks
Hema

Comment 3

[Punam Dahiya [:pdahiya]]

Able to replicate the issue on flmae-kk todays m-c build (BuildId:20141014040203) after closely observing only the last 15 seconds of video attached. Here are more precise STR to replicate the issue.

1. Open video app (close video app if it's previously opened and disable screen lock in settings)
2. Click on any other video except first in video thumbnail list.
3. Let the video play for few seconds ( ~ 3 sec)
4. Press power button before playing video ends.
5. Resume by pressing power again.
6. Video player shows the a different video where as title and slider time stays same.

Looking into the issue. Thanks

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted for branch checks.

Comment 5

[Punam Dahiya [:pdahiya]]

Findings so far, this bug can be replicated by pressing home button in Step 4 and 5 in #comment 3. When player is hidden on visibility change we are releasing video. When the player is brought back in focus, value of currentVideo variable changes and restoreVideo sets the wrong video url. Adding log statements in setVideoUrl shows a different video name (first video in thumbnail list).

https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L114
https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L1266
https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L901

Looking into why currentVideo variable is getting reset to first video in thumbnail list on visibility change.

Comment 6

[Punam Dahiya [:pdahiya]]

Debugged some more and currentVideo is getting set to first thumbnaillist inside updateLoadingSpinner when player is hidden. 

https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L676

Comment 7

[Punam Dahiya [:pdahiya]]

updateLoadingSpinner is getting called when first scan ends causing currentVideo to reset to first video in thumbnail list, 
https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/db.js#L50

Comment 8

[Punam Dahiya [:pdahiya]]

My investigation so far narrows down the reason of this bug to reset of currentVideo inside updateLoadingSpinner. https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L676

This code has been there since bug 903920 which means we should see this issue in branches 1.4, 2.0, 2.1 and 2.2. This issue can be replicated only when a user starts viewing a video before the first scan ends and than immediately suspend video by pressing power or home key. 
 
Now that has a low chance of occurrence and probably the reason it was not caught before. I will attach the patch with the fix that is bringing currentVideo reset inside the check for tablet and landscape mode (line 677).

It will help if QA can validate the branches this issue is replicable in (#comment 4).

Comment 9

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of bug 1082555

Hi Russ
Please review attached PR which checks for tablet and landscape before resetting value inside currentVideo global variable. Thanks

Comment 10

[Punam Dahiya [:pdahiya]]

Setting NI flag for John who is more familiar with code in updateLoadingSpinner, to check if this change is going to impact video player in tablet. Thanks

Comment 11

[Russ Nicoletti [:russn]]

Comment on attachment 8505749 [details] [review]
PR with fix of bug 1082555

That change looks correct to me. I'm giving r+, probably should wait to hear from John before landing (even though I think it's highly unlikely he will disagree with the change).

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Punam,

Thanks for this. This is a regression made by the tablet patch. Sorry about this. I feel we still need to initialize currentVideo even it is in tablet portrait mode. User may rotate device without tapping any video. I had left few comments in PR. Please fine them there.

Comment 13

[Chris Kreinbring [:CKreinbring]]

The bug repros on Flame 2.2, Flame 2.1, Flame 2.0 and Flame 2.0 base.  Engineering builds and shallow flashes were used.
Actual result: Sometimes, when a video is launched the title for the top video in the list will appear momentarily in the title bar.  If the user then taps the Home button then relaunches the video, the top video in the list will appear instead of the video that was previously started.

Flame 2.2
BuildID: 20141016070843
Gaia: 5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586
Gecko: 77f3ca1fe052
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Platform Version: 36.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Flame 2.1
BuildID: 20141015143144
Gaia: 477a9e61c3edf12f32a62a19d329cd277202cc6b
Gecko: 67573e422a0f
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Platform Version: 34.0
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.0
BuildID: 20141016082526
Gaia: c6c6116ca225c2c934220ae6867e5a3256d65e00
Gecko: b19fe9009a7c
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Platform Version: 32.0
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Flame 2.0 Base
BuildID: 20140904160718
Gaia: 506da297098326c671523707caae6eaba7e718da
Gecko: 
Platform Version: 32.0
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 14

[Punam Dahiya [:pdahiya]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #12)
> Hi Punam,
> 
> Thanks for this. This is a regression made by the tablet patch. Sorry about
> this. I feel we still need to initialize currentVideo even it is in tablet
> portrait mode. User may rotate device without tapping any video. I had left
> few comments in PR. Please fine them there.

Thanks John, I updated the PR with your feedback. Setting feedback flag for you before landing the patch in master.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8505749 [details] [review]
PR with fix of bug 1082555

Thanks. I think this is good.

Comment 16

[Punam Dahiya [:pdahiya]]

Thanks John, Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/8ffcc7938e0749a55ca39a7c5aa22757df3a9e8a

Comment 17

[Punam Dahiya [:pdahiya]]

Comment on attachment 8505749 [details] [review]
PR with fix of bug 1082555

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):Bug 903920
[User impact] if declined:  When a user starts viewing a video before the first scan ends and than immediately suspend video by pressing power or home key, on coming back to video player, user will see a different video (first video in thumbnail list) loaded in the player.

[Testing completed]: on Master
[Risk to taking this patch] (and alternatives if risky): None, one line patch that checks if currentVideo has a value before resetting it to first video in thumbnail list.
[String changes made]: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/734e796e5858c67b09c1ef3f39b7bf2cf3bfde82

Comment 19

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.2, 2.1 (Full Flash, nightly). 

Actual Results: Video titles behave correctly after user locks phone and selects a different video thumbnail. 

Device: Flame Master
Build ID: 20141022040201
Gaia: 4d7f051cede6544f4c83580253c743c22b0cb279
Gecko: ae4d9b4ff2ee
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
Build ID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Version: 34.0 (2.1)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 20

[KTucker [:KTucker][Inactive 3/4/2016]]

Leaving verifyme keyword in case this is uplifted to 2.0

Comment 21

[Mike Lien[:mlien]]

set 2.0? to see if it needs to be uplifted to 2.0

Comment 22

[Rachelle Yang [:ryang][[e-mail]]]

no partner is requested this on 2.0 for now; suggesting to denominate it for now...
Thanks!

Comment 23

[Mike Lien[:mlien]]

refer to comment 22, mark v2.0 status as wontfix