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

Status: RESOLVED FIXED

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

Description

[Leo]

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 :

Comment 1

[Leo]

Attachment: Pointer to pull request

Comment 2

[Leo]

Attachment: Pointer to pull request

Comment 3

[Wayne Chang [:wchang]]

Hi David, was there a reason why this was deliberately done (setting slider at 25% each time)?

Comment 4

[David Flanagan [:djf]]

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.

Comment 5

[David Flanagan [:djf]]

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.

Comment 6

[Alex Keybl [:akeybl]]

The user impact here is low, but we'd accept a low risk fix if nominated for uplift (approval-gaia-v1? on the patch)

Comment 7

[[e-mail]]

take it

Comment 8

[[e-mail]]

Attachment: set a flag during this thumbnail creation process to tell the slider not to move.

Comment 9

[David Flanagan [:djf]]

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.

Comment 10

[[e-mail]]

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+?

Comment 11

[Dylan Oliver [: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.

Comment 12

[Dylan Oliver [:doliver]]

Fred, can you describe the fix you made? David proposed a couple of options in comment #4.

Comment 13

[Dylan Oliver [:doliver]]

(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+.

Comment 14

[[e-mail]]

The fix follow david's suggestion,
to pass a playEnded flag during the thumbnail creation process to tell the slider not to move.

Comment 15

[Dylan Oliver [:doliver]]

Marking needinfo to :djf so that he sees comment #11 -- this review is a priority due to the QE1 milestone.

Comment 16

[David Flanagan [:djf]]

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.

Comment 17

[Alex Keybl [:akeybl]]

Clearing the target milestone if that's confusing - we'd already agreed this wouldn't block.

Comment 18

[Dylan Oliver [:doliver]]

Leo, can you take over this bug since it is a P3?

Comment 19

[Leo]

Yes, I am already working on this issue. I will take this bug.

Comment 20

[Leo]

Attachment: Pointer to pull request

Changes have been as per djf comments. Please review the code.

Comment 21

[David Flanagan [:djf]]

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.

Comment 22

[[e-mail]]

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.

Comment 23

[David Flanagan [:djf]]

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.

Comment 24

[[e-mail]]

Thanks for feedback.
Updated comments and passed the jslint check.

merged to master-gaia 31fd830aa7c3a8d6e0239f1baabb6e5d92db6367

Comment 25

[David Flanagan [:djf]]

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.

Comment 26

[Leo]

David,

I can't see the fix either on master or v1-train branches. Can you please confirm these changes are merged?

Comment 27

[Leo]

This patch is merged to V1-train