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)
Tracking
(b2g-v2.0 wontfix, b2g-v2.1 verified, b2g-v2.2 verified)
VERIFIED
FIXED
2.1 S7 (24Oct)
People
(Reporter: poojas, Assigned: pdahiya)
References
Details
(Whiteboard: [caf priority: p2][CR 739314])
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/x-github-pull-request
|
rnicoletti
:
review+
johnhu
:
feedback+
fabrice
:
approval-gaia-v2.1+
|
Details |
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?
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
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
Blocks: CAF-v2.1-CC-metabug
No longer depends on: CAF-v2.1-FC-metabug
Updated•10 years ago
|
Whiteboard: [CR 739314]
Updated•10 years ago
|
Whiteboard: [CR 739314] → [caf priority: p2][CR 739314]
Updated•10 years ago
|
Blocks: CAF-v2.1-FC-metabug
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
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
Assignee | ||
Comment 8•10 years ago
|
||
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).
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
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)
Updated•10 years ago
|
QA Contact: ckreinbring
Comment 13•10 years ago
|
||
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?]
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8505749 -
Flags: feedback?(im)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Contact: ckreinbring
No longer blocks: CAF-v2.1-FC-metabug
Updated•10 years ago
|
Whiteboard: [caf priority: p2][CR 739314] → [caf priority: p2][CR 739314][ready-to-land]
Comment 15•10 years ago
|
||
Comment on attachment 8505749 [details]
PR with fix of bug 1082555
Thanks. I think this is good.
Attachment #8505749 -
Flags: feedback?(im) → feedback+
Assignee | ||
Comment 16•10 years ago
|
||
Thanks John, Patch landed on master
https://github.com/mozilla-b2g/gaia/commit/8ffcc7938e0749a55ca39a7c5aa22757df3a9e8a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [caf priority: p2][CR 739314][ready-to-land] → [caf priority: p2][CR 739314]
Target Milestone: --- → 2.1 S7 (24Oct)
Updated•10 years ago
|
Attachment #8505749 -
Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
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)
Comment 20•10 years ago
|
||
Leaving verifyme keyword in case this is uplifted to 2.0
Comment 21•10 years ago
|
||
set 2.0? to see if it needs to be uplifted to 2.0
blocking-b2g: 2.1+ → 2.0?
Comment 22•10 years ago
|
||
no partner is requested this on 2.0 for now; suggesting to denominate it for now...
Thanks!
blocking-b2g: 2.0? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•