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)
Firefox OS Graveyard
Gaia::Video
Tracking
(blocking-b2g:koi+, 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().
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
Hi Marco and Cheng Luo,
I think I am the one who need to help this.
Flags: needinfo?(ehung)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → johu
Assignee | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=johnhu]
Assignee | ||
Updated•11 years ago
|
Assignee: johu → nobody
Assignee | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=johnhu] → [good-first-bug][mentor=johnhu][mentor-lang=zh]
Updated•11 years ago
|
blocking-b2g: --- → koi?
Assignee | ||
Comment 5•11 years ago
|
||
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]
Assignee | ||
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
QA Wanted - Can someone check if this reproduces on 1.1 or not?
Keywords: qawanted
Updated•11 years ago
|
QA Contact: sparsons
Comment 8•11 years ago
|
||
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
Updated•11 years ago
|
Keywords: regression
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Comment 9•11 years ago
|
||
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-
Assignee | ||
Comment 10•11 years ago
|
||
This bug is no longer blocking flatfish because it is a general bug.
Assignee | ||
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
merged to master:
https://github.com/mozilla-b2g/gaia/commit/fe33351a4a460172212ad234b1a98ba2dcd29ac4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 15•11 years ago
|
||
Can you merge this patch to v1.2? Thanks.
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
Flags: needinfo?(johu)
Assignee | ||
Updated•11 years ago
|
status-b2g-v1.2:
--- → fixed
Reporter | ||
Comment 18•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #17)
> merged to v1.2:
>
Has it been merged to v1.2f ?
Flags: needinfo?(styang)
Updated•11 years ago
|
Whiteboard: [Fugu] [v1.2f-uplift-needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•