Closed Bug 1082555 Opened 10 years ago Closed 10 years ago

video clips and there title gets interchanged during suspend resume operation

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.0 wontfix, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S7 (24Oct)
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: poojas, Assigned: pdahiya)

References

Details

(Whiteboard: [caf priority: p2][CR 739314])

Attachments

(2 files)

Attached file steps to reproduce (deleted) —
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"
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
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
Assignee: nobody → pdahiya
blocking-b2g: 2.1? → 2.1+
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
Adding qawanted for branch checks.
Keywords: qawanted
No longer depends on: CAF-v2.1-FC-metabug
Whiteboard: [CR 739314]
Whiteboard: [CR 739314] → [caf priority: p2][CR 739314]
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.
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
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
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).
Attached file PR with fix of bug 1082555 (deleted) —
Hi Russ Please review attached PR which checks for tablet and landscape before resetting value inside currentVideo global variable. Thanks
Attachment #8505749 - Flags: review?(rnicoletti)
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
Flags: needinfo?(im)
Comment on attachment 8505749 [details] 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).
Attachment #8505749 - Flags: review?(rnicoletti) → review+
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.
Flags: needinfo?(im)
QA Contact: 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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
(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.
Attachment #8505749 - Flags: feedback?(im)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Contact: ckreinbring
No longer blocks: CAF-v2.1-FC-metabug
Whiteboard: [caf priority: p2][CR 739314] → [caf priority: p2][CR 739314][ready-to-land]
Comment on attachment 8505749 [details] PR with fix of bug 1082555 Thanks. I think this is good.
Attachment #8505749 - Flags: feedback?(im) → feedback+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 8505749 [details] 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
Attachment #8505749 - Flags: approval-gaia-v2.1?(bbajaj)
Whiteboard: [caf priority: p2][CR 739314][ready-to-land] → [caf priority: p2][CR 739314]
Target Milestone: --- → 2.1 S7 (24Oct)
Attachment #8505749 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Leaving verifyme keyword in case this is uplifted to 2.0
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Keywords: verifyme
set 2.0? to see if it needs to be uplifted to 2.0
blocking-b2g: 2.1+ → 2.0?
no partner is requested this on 2.0 for now; suggesting to denominate it for now... Thanks!
blocking-b2g: 2.0? → ---
refer to comment 22, mark v2.0 status as wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: