Closed Bug 938967 Opened 11 years ago Closed 11 years ago

[fugu][hamachi]The video had be sought to 00:00 could not be played from background to foreground

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: cheng.luo, Assigned: johnhu)

Details

(Keywords: regression, Whiteboard: [Fugu] [v1.2f-uplift-needed])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.19 (KHTML, like Gecko) Ubuntu/11.04 Chromium/18.0.1025.168 Chrome/18.0.1025.168 Safari/535.19 Steps to reproduce: 1、launch video app,select anyone file to play from list; 2、pause,seek to 00:00 (necessary); 3、click “HOME”,return to homescreen; 4、swtich video app to foreground; Actual results: could not to play after clicking "play" button and blackscreen in videoframe Expected results: play normally
in video.js: when swtich to background,it save restoreTime in releaseVideo(), then swtich to foreground,it seek to restoreTime in restoreVideo(). But if restoreTime is 00:00, it directly return in restoreVideo().
Hi Evelyn, Based on Comment 1, it seems there is a edge case which didn't be considered well in video app. Could you suggest gaia video guys to help this? Thanks.
Flags: needinfo?(ehung)
Hi Marco and Cheng Luo, I think I am the one who need to help this.
Flags: needinfo?(ehung)
Assignee: nobody → johu
The examination result by Cheng Luo is correct direction to fix this one. We only checks if the restoreTime has a value instead of checking type. That's the main cause. It's a good first bug. I will ask somebody to watch this. If no one found, I can fix this by myself.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][mentor=johnhu]
Assignee: johu → nobody
Whiteboard: [good first bug][mentor=johnhu] → [good-first-bug][mentor=johnhu][mentor-lang=zh]
blocking-b2g: --- → koi?
Since it becomes koi?, I pick this bug and make the patch for this.
Assignee: nobody → johu
Whiteboard: [good-first-bug][mentor=johnhu][mentor-lang=zh]
Attached file patch for this bug. (deleted) —
This bug is caused by the checking code of restoreTime. It should return when the value is null or undefined. But it also returns when the value is 0. This checking code is used at the extreme case that user opens the player view and presses home immediately. It should move to the timing setting player's currentTime. When we meet this extreme case, we should play the video based on the metadata information of currentVideo.
Attachment #8334370 - Flags: review?(dflanagan)
QA Wanted - Can someone check if this reproduces on 1.1 or not?
Keywords: qawanted
QA Contact: sparsons
This issue does not reproduce on the Leo 1.1 Build ID: 20131119041201 Gaia b585b32441fafa67f2b4582db23be5f3a2afab21 SourceStamp 7c3cfc0936ca BuildID 20131119041201 Version 18.0 -Regression Window for Buri 1.2- Issue started to occur on the 1.2 Build ID: 20130911040201 Gaia ebbb325958c66c3e68e1f190472d5f6e9076d83a SourceStamp f9e8e8ce552c BuildID 20130911040201 Version 26.0a1 Last working 1.2 Build ID: 20130910040201 Gaia 6deda9d7c51f278443f704217eaed722044a03e7 SourceStamp be1053dc223b BuildID 20130910040201 Version 26.0a1
Keywords: qawanted
Keywords: regression
blocking-b2g: koi? → koi+
Blocks: 903920
Comment on attachment 8334370 [details] patch for this bug. The patch seems more complicated than the description of the bug here. I left a bunch of github comments because I didn't really understand what you were doing with some of it. The code probably works fine, but I'm wondering if you can simplify the patch to minimize risk. Or at least add enough comments so I can understand it.
Attachment #8334370 - Flags: review?(dflanagan) → review-
No longer blocks: 903920
This bug is no longer blocking flatfish because it is a general bug.
Comment on attachment 8334370 [details] patch for this bug. Hi David, Thanks for your review. I had done some modification according to your concerns. I had replied some of your comments to github and put more comments on the code. The main thought of this bug is to handle the case of restoreTime is null or undefined which has two extreme cases, I feel it's seldom to have them: 1. The first extreme case is that user taps on a unwatched video, presses home immediately. In this case, the dom.player may not finish the loading of video and the restoreTime is null. At the same time, the currentTime of metadata is still undefined because we haven't updateMetadata. 2. The second extreme case is that user plays a video correctly, presses home(restoreTime is set), opens video app and presses home immediately(restoreTime is not set). In this case, the player.readyState is 0. So, we don't need to update the restoreTime and remain it as its original value. When user opens video app again, it restores to the first restoreTime and that's as expected. That's why I don't want to set restoreTime to null when player.readyState is 0. Please review it. Thanks.
Attachment #8334370 - Flags: review- → review?(dflanagan)
Comment on attachment 8334370 [details] patch for this bug. Thanks for the explanation, and thanks for thinking through the corner cases. r+ to land this if you're confident that currentVideo.metadata will always be defined when restoreVideo() is called. (i.e. that currentVideo will never be a blob from an activity request.) In general, it is so hard to handle these state transition cases where we can't be sure what flags were set when. It leads to complicated logic and I wish there was some kind of clear methodology that would help us to write solid code for stuff like this. I tend to think about finite state machines: enumerate all possible states and all possible events. But it tends to result in code that is harder to understand...
Attachment #8334370 - Flags: review?(dflanagan) → review+
Thanks for reviewing. I am confident currentVideo.metadata is defined because we list the thumbnail with video's metadata. If there is a video without metadata, that will be filtered out at listing. And currentVideo will always not be a blob/url from an activity because we use view.html/view.js to handle open/view activity.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Can you merge this patch to v1.2? Thanks.
I was not able to uplift this bug to v1.2. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1.2 git cherry-pick -x -m1 fe33351a4a460172212ad234b1a98ba2dcd29ac4 <RESOLVE MERGE CONFLICTS> git commit
Flags: needinfo?(johu)
(In reply to John Hu [:johnhu] from comment #17) > merged to v1.2: > Has it been merged to v1.2f ?
Flags: needinfo?(styang)
This is on v1.2f
Flags: needinfo?(styang)
Whiteboard: [Fugu] [v1.2f-uplift-needed]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: