[Video Player] After completion of video playing slider goes to 25% of total video time before resetting to zero

RESOLVED FIXED

Status

P3
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: gasolin)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:-, b2g18+ fixed)

Details

(Whiteboard: [TD-9218])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
1. Title : After completing video play slider goes to 25% of total video time before resetting to zero
2. Precondition : Video player running
3. Tester's Action : watching till end of video file
4. Detailed Symptom (ENG.) : After completing video play slider goes to 25% of total video time before resetting to zero
5. Gaia Revision # :   "8e7262e3d98ea9574d7f79c1e890ad80cfa40c27"
6. Expected : After completion video playing slider should return to zero.
7.Reproducibility: Yes
                1)Frequency Rate : 100%
8.Comparison Results : 
                1)Model Comparing : 
9. Attached files: 
                1)Log : 
                2)Test Contents : 
                3)Video file :
(Reporter)

Updated

6 years ago
blocking-b2g: --- → leo?
(Reporter)

Comment 1

6 years ago
Created attachment 735171 [details] [diff] [review]
Pointer to pull request
Assignee: nobody → leo.bugzilla.gaia
Attachment #735171 - Flags: review?(dflanagan)
(Reporter)

Comment 2

6 years ago
Created attachment 735173 [details]
Pointer to pull request
Attachment #735171 - Attachment is obsolete: true
Attachment #735171 - Flags: review?(dflanagan)
Attachment #735173 - Flags: review?(dflanagan)
Hi David, was there a reason why this was deliberately done (setting slider at 25% each time)?
Flags: needinfo?(dflanagan)
The video app displays a thumbnail of each video.  If you watch part and stop, it displays the current frame as kind of a bookmark and remembers where to resume.

But if you're at the start of a video (or reach the end and go back to the start) it does not take a thumbnail of the very start of the video because that is often a black frame.  So it chooses 25%. (I'm not sure that is a good choice... 10s or 25% whichever is smaller might be better).

It should be pretty easy to set a flag during this thumbnail creation process to tell the slider not to move.  Actually the easier thing is probably just to switch back to the thumbnail list before capuring the new thumbnail image.  That used to be tricky because of the way we handled fullscreen, but it might be straightforward now.
Flags: needinfo?(dflanagan)
Comment on attachment 735173 [details]
Pointer to pull request

I think this patch will break the way that the app captures a thumbnail image.  See my comment above.
Attachment #735173 - Flags: review?(dflanagan) → review-

Comment 6

6 years ago
The user impact here is low, but we'd accept a low risk fix if nominated for uplift (approval-gaia-v1? on the patch)
blocking-b2g: leo? → -
tracking-b2g18: --- → +
(Reporter)

Updated

6 years ago
Whiteboard: [TD-9218]
Target Milestone: --- → Leo QE1 (5may)
(Assignee)

Comment 7

6 years ago
take it
Assignee: leo.bugzilla.gaia → gasolin
(Assignee)

Comment 8

6 years ago
Created attachment 738688 [details]
set a flag during this thumbnail creation process to tell the slider not to move.
Attachment #738688 - Flags: review?(dflanagan)
According to https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.gaia/E1xniLLxPoc this bug doesn't have high enough priority for me to review the patch right now.  Sorry!

Let me know if it gets marked leo+ and I'll make it a priority to review.
(Assignee)

Comment 10

6 years ago
David, sorry I did not noticed the flag but I think this bug is in the highest
priority leo+ bugs list: http://is.gd/kJk1vk

Dylan, not sure why this bug in Leo QE1 list but not leo+?
blocking-b2g: - → leo?
Flags: needinfo?(doliver)
We do want to prioritize the reviews of Leo QE1 bugs -- this fits condition 2 on the post that David referenced in comment #9, blocking a partner milestone.
Flags: needinfo?(doliver)
Fred, can you describe the fix you made? David proposed a couple of options in comment #4.
Flags: needinfo?(gasolin)
(In reply to Fred Lin [:gasolin] from comment #10)
> Dylan, not sure why this bug in Leo QE1 list but not leo+?

We are still working through some of the process here, but this one is tracking+ and will be approved for landing on v1-train with an r+.
(Assignee)

Comment 14

6 years ago
The fix follow david's suggestion,
to pass a playEnded flag during the thumbnail creation process to tell the slider not to move.
Flags: needinfo?(gasolin)
Marking needinfo to :djf so that he sees comment #11 -- this review is a priority due to the QE1 milestone.
Flags: needinfo?(dflanagan)
Comment on attachment 738688 [details]
set a flag during this thumbnail creation process to tell the slider not to move.

This patch does the same thing as the last one: it just prevents the app from seeking ahead to take a good thumbnail for the video.  It doesn't prevent the slider from moving as the description says, it just doesn't seek.

Instead, I suggest that you set a flag in captureFrame() and clear it in the nested doneSeeking() function.  Then, in timeUpdated(), you ignore the even if that flag is set.

That way, the video app can still get the thumbnail image it wants, but the controls won't move while it does it.
Attachment #738688 - Flags: review?(dflanagan) → review-
Flags: needinfo?(dflanagan)
Priority: -- → P3
Clearing the target milestone if that's confusing - we'd already agreed this wouldn't block.
blocking-b2g: leo? → -
Target Milestone: Leo QE1 (5may) → ---
Leo, can you take over this bug since it is a P3?
Assignee: gasolin → leo.bugzilla.gaia
Flags: needinfo?(leo.bugzilla.gaia)
(Reporter)

Comment 19

6 years ago
Yes, I am already working on this issue. I will take this bug.
Flags: needinfo?(leo.bugzilla.gaia)
(Reporter)

Comment 20

6 years ago
Created attachment 741257 [details]
Pointer to pull request

Changes have been as per djf comments. Please review the code.
Attachment #735173 - Attachment is obsolete: true
Attachment #741257 - Flags: review?(dflanagan)
Flags: needinfo?(dflanagan)
Comment on attachment 741257 [details]
Pointer to pull request

See my comments on github.

This patch doesn't fix the bug correctly because again, it just resets the play time to zero while the app is trying to capture the frame.  Please use the new flag just to ignore the time change event.

If you don't understand what I mean, just assign the bug to me and I'll fix it.
Attachment #741257 - Flags: review?(dflanagan) → review-
Flags: needinfo?(dflanagan)
(Assignee)

Comment 22

6 years ago
Comment on attachment 738688 [details]
set a flag during this thumbnail creation process to tell the slider not to move.

rename and change flag place by djf latest comment.
It fix both start from 0 and after completion status.
Attachment #738688 - Flags: review- → review?(dflanagan)
(Assignee)

Updated

6 years ago
Assignee: leo.bugzilla.gaia → gasolin
Comment on attachment 738688 [details]
set a flag during this thumbnail creation process to tell the slider not to move.

The code is good, but the lines are too long and cause lint errors. Please fix the two lint errors and also update the code comments, as I describe on github.

Once you do that, this code is ready to land.  Email me (or set needinfo on this bug) if you need me to land it for you.
Attachment #738688 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 24

6 years ago
Thanks for feedback.
Updated comments and passed the jslint check.

merged to master-gaia 31fd830aa7c3a8d6e0239f1baabb6e5d92db6367
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
This patch got merged into a patch for bug 856542, which then got uplifted to v1-train, so I'm setting the status-b2g18 flag to indicate that this is now fixed on v1-train as well as on master.
status-b2g18: --- → fixed
(Reporter)

Comment 26

6 years ago
David,

I can't see the fix either on master or v1-train branches. Can you please confirm these changes are merged?
Flags: needinfo?(dflanagan)
(Reporter)

Comment 27

5 years ago
This patch is merged to V1-train
Flags: needinfo?(dflanagan)
You need to log in before you can comment on or make changes to this bug.