Closed Bug 865122 Opened 7 years ago Closed 7 years ago

[A/V] is it possible to show buffering popup in the case of streaming?

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 verified)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
b2g18 --- verified

People

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

Details

(Whiteboard: mentoredbug mentor=djf [TD-25210] c=video , MiniWW)

Attachments

(3 files, 1 obsolete file)

Contact: hjlee7111@gmail.com

Build ID : 20130419115238

When the device is connected by WIFI, I think there's no problem like this.
However, in 3G connection case, almost every streaming fills like freezing because app doesn't show any notification to user.
Application removes loading animation after it successfully load the video clip.
But then real buffering starts and user cannot figure out that streaming keeps going or it's stopped.
It fills like freezing...sometimes more than 20s.
(sometimes it's working...sometime it's stuck. I'm checking stuck case)

I think application should shows some information to user.
blocking-b2g: --- → leo?
djf what would be your feedback on this ?
blocking-b2g: leo? → -
Flags: needinfo?(dflanagan)
it would be better some indicator, which is runninng (ex, some anication) in during buffering on 3G.
currently there are no indicator for it is buffering. so it looks like ui freezing.
When the video app runs as an app, it only plays videos from the sdcard, so I assume this bug is about the video app when used to play youtube and similar video, right?

The HTML <video> tag is supposed to send us events when playback stops because of buffering.  I haven't used them, but they are there, and if they work right, that is the functionality we'd need to give feedback to the user.

I think this would be an easy (1 day) bug to fix. We need some input from UX if they want us to do anything other than just display the normal spinner again during buffering. If this is a priority for our partners (or becomes a priority for us) feel free to assign it to me.

(Note that we might have the same issue with the plain <video> tag on web pages. When a plain <video> element is displaying on a web page and is showing its own controls, maybe it should have the same kind of buffering indicator. That would be a gecko fix that would benefit Firefox for Android as well...)

Asking Peter for UX suggestions here because he and I have worked on other video-related UX bugs.
Flags: needinfo?(dflanagan) → needinfo?(pla)
Hi David,

Please refer to this guide for activity/progress indication:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/UX/Building_blocks/Progress_and_activity

I think what we need here is the non-modal activity or progress bar that sits at or near the top of the screen.  If the amount of data to be buffered is known, use the progress bar, if it is unknown, use the activity bar.  The spinner is supposed to be for sending data, while the activity/progress bars are for receiving data.

There is also this resource that may help:
http://buildingfirefoxos.com/building-blocks/progress-and-activity/

A building block should be used here, but if for whatever reason you need the visual/animated assets, let me know and I can provide it for you.

Peter.
Flags: needinfo?(pla)
(In reply to David Flanagan [:djf] from comment #3)
> When the video app runs as an app, it only plays videos from the sdcard, so
> I assume this bug is about the video app when used to play youtube and
> similar video, right?
> 
> The HTML <video> tag is supposed to send us events when playback stops
> because of buffering.  I haven't used them, but they are there, and if they
> work right, that is the functionality we'd need to give feedback to the user.
> 
> I think this would be an easy (1 day) bug to fix. We need some input from UX
> if they want us to do anything other than just display the normal spinner
> again during buffering. If this is a priority for our partners (or becomes a
> priority for us) feel free to assign it to me.
> 
> (Note that we might have the same issue with the plain <video> tag on web
> pages. When a plain <video> element is displaying on a web page and is
> showing its own controls, maybe it should have the same kind of buffering
> indicator. That would be a gecko fix that would benefit Firefox for Android
> as well...)
> 
> Asking Peter for UX suggestions here because he and I have worked on other
> video-related UX bugs.

Yes, this is only for streaming case.
In the case of streaming, activiy or progress bar is enough to inform user that something keeps working.
I changed Flag to leo+ to check this issue in triage again.
blocking-b2g: - → leo?
It seems that we have two ways to solve this issue, the easy one that would satisfy the reporter and the other one more complicated. We request UX and product input in order to decide.
Flags: needinfo?(jcarpenter)
Flags: needinfo?(jachen)
Flags: needinfo?(ffos-product)
If the amount of data to be buffered
> is known, use the progress bar, if it is unknown, use the activity bar.  The
> spinner is supposed to be for sending data, while the activity/progress bars
> are for receiving data.

Because the data is likely not known in this case, I would go with the activity bar building block per Peter's email.

I hope that is the easy answer that David was looking for. 

What djf says here:
(Note that we might have the same issue with the plain <video> tag on web pages. When a plain <video> element is displaying on a web page and is showing its own controls, maybe it should have the same kind of buffering indicator. That would be a gecko fix that would benefit Firefox for Android as well...)
>> That does make sense for the future and perhaps David can make a bug to ensure its explained properly...?
Flags: needinfo?(jcarpenter)
Flags: needinfo?(jachen)
blocking-b2g: leo? → leo+
Whiteboard: mentoredbug mentor=djf
(In reply to jachen from comment #8)
> Because the data is likely not known in this case, I would go with the
> activity bar building block per Peter's email.

+1
Flags: needinfo?(ffos-product)
Whiteboard: mentoredbug mentor=djf → mentoredbug mentor=djf [TD-25210]
Assignee: nobody → mshiao
Hi Peter,

A quick screenshot to show current placement of the activity bar.  Let me know if it works for you.  

Thanks,
Mark
Attachment #748750 - Flags: review?(pla)
Attachment #748750 - Flags: review?(pdolanjski)
Target Milestone: --- → 1.1 QE2
Priority: -- → P1
Mark,

Is that the same activity bar that the video app uses when scanning the SD card?

I had imagined that we'd want something more in the middle of the screen, on top of the frozen video content, or something that was integrated with the player controls somehow. (Like if we're buffering, ensure that the controls are shown and then do that animation in the timeline/scrubber thing.)

But Peter would be the right person to decide about that, of course.
Whiteboard: mentoredbug mentor=djf [TD-25210] → mentoredbug mentor=djf [TD-25210] c=video
(In reply to David Flanagan [:djf] from comment #11)
> Mark,
> 
> Is that the same activity bar that the video app uses when scanning the SD
> card?
> 
> I had imagined that we'd want something more in the middle of the screen, on
> top of the frozen video content, or something that was integrated with the
> player controls somehow. (Like if we're buffering, ensure that the controls
> are shown and then do that animation in the timeline/scrubber thing.)
> 
> But Peter would be the right person to decide about that, of course.

Hi David,

Yes, it is the same animation used during card scan. I quickly implemented the activity bar as suggested by some of the folks but will definitely wait for Peter's suggestions/comments :)
Hi Mark,

Please let me know the status of this issue fix.
(In reply to Leo from comment #13)
> Hi Mark,
> 
> Please let me know the status of this issue fix.

Hi Leo,

I'm waiting for Peter's feedback on layout.  I've attached a mock for this proposal in comment #10.
Hi, Mark is still waiting on UX review on the screenshot.
Flags: needinfo?(firefoxos-ux-bugzilla)
If this is a priority bug for the San Diego work week, Dominic and I can probably steal it from Mark and make something work.  I'd suggest that we animate the timeline slider rather than reusing the scanning indicator.  Or, even simpler, we could just put a spinner right in the middle of the screen.
Reassigning from general FFOS-UX to Casey. Casey, this is just a quick final layout review; extensive, prior UX was done on this.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(kyee)
Whiteboard: mentoredbug mentor=djf [TD-25210] c=video → mentoredbug mentor=djf [TD-25210] c=video , MiniWW
Looks good to me. We should land this and we can do further UX tweaks in a follow-up bug. Andreas
Taking this from Mark. We'll land something sensible and UX can file a new bug to tweak the UX if needed.
Assignee: mshiao → dflanagan
If we are dealing with <video> elements on a page, wouldn't it make sense to have the progress animation (spinner) on the element itself and not at the top of the page?   

I can see how this could be confused for page content loading.
Flags: needinfo?(kyee)
Comment on attachment 748750 [details]
Screenshot of video buffering activity bar

Removing review requests.
Attachment #748750 - Flags: review?(pla)
Attachment #748750 - Flags: review?(pdolanjski)
According to the HTML spec, the "waiting" and "playing" events sound like exactly what we need for showing and hiding the spinner or progress bar or whatever. But it looks like they're not implemented (at least not in gecko 18)
Please ignore comment 22. I was adding tests to the wrong file.
Attached file Pointer to pull request (obsolete) —
Hi David,

Currently when video streaming is happening, there is no indication/information to user.

In this patch:
1. Added spinner when "waiting" and "videoStalled" (network loading) events fired.
2. Spinner will be hidden when "canplay" is fired.

Please review the patch.
Attachment #755846 - Flags: review?(dflanagan)
Comment on attachment 755846 [details]
Pointer to pull request

This is a good first step, but the patch needs more work, I think.  See comments on github. And if you plan to keep working on it, please steal it from me and re-assign to yourself.
Attachment #755846 - Flags: review?(dflanagan) → review-
Assignee: dflanagan → leo.bugzilla.gaia
Attachment #758330 - Flags: review?(dflanagan)
Hi @eshapiro,

I have tested your patch on leo device using 2G network and my observation is: spinner is not showing sometimes when video gets freezed.

Hi David,

I have worked as per your suggestions and comments and made a patch. As per my testing, the patch seems to be working. Please let me know if I can upload the patch for your review.
Leo,

You don't have to ask before attaching a patch. The deadline for this bug is tomorrow, so please attach your patch as soon as possible.

I like eshapiro's patch a lot, but if it isn't working correctly we can't land it. Do you know what the difference between your patch and his is? What situation is not being caught by his patch?
Flags: needinfo?(leo.bugzilla.gaia)
Leo or Evan: take a look at bug 870564 to see whether the fix for that has any impact on this bug.
Mike Habicher writes: " it's possible the inconsistent states in the media/netwerk layers were preventing a waiting event from being properly sent to JS."

Leo: can you test eshapiro's patch again with the patch for bug 870564 applied? (It is a gecko patch that just landed on b2g18 today).  But please attach your own patch first!
Hi David,

Please review the patch.
This implementation is almost same as the one used for embedded video player (in gecko toolkit).

I will test eshapiro's patch with the patch appied for bug 870564 and update you the results.
Attachment #755846 - Attachment is obsolete: true
Attachment #758962 - Flags: review?(dflanagan)
Flags: needinfo?(leo.bugzilla.gaia)
Comment on attachment 758962 [details]
Pointer to pull request

r-.

I can't accept a patch this complicated without detailed comments explaining what the code does and why such complicated code is necessary in this case.  Also, as noted on github, this patch registers listeners for events that it then ignores.

I don't actually want detailed comments, though: I'd much prefer to see a simpler patch.
Attachment #758962 - Flags: review?(dflanagan) → review-
Comment on attachment 758330 [details]
link to github pull request

I'm giving an r+ on this if you fix a few nits:

In my testing, I always see a playing event before the canplaythrough, so I don't see why that is needed.  (Though I haven't tested without it).

Also, please change the spinner overlay to make it fully transparent (as in Leo's patch).

I'm not convinced that this patch is enough to fully resolve the bug, but it is a step in the right direction, so I think we ought to land it. The trouble I'm having is that my wifi is too fast to have youtube stalls, and my data connection (2G) is too slow to play much of anything at all.  Basically video is unusable over a 2G connection both with or without this patch.

I think there may be other, unrelated bugs in view.js that are interacting with this bug.  When I test, I sometimes see the time slider jump to the end of the video, for example.  And when I click on the pause button while the spinner is displayed, it does not seem to do anything.

View.js has completely separate video player code from what is in video.js, so it could be that bugs have been fixed in video.js without corresponding updates to view.js.  Evan, if you've got time to dig into this some more, that would be great.  But at a minimum, let's at least get your patch tweaked and landed.
Attachment #758330 - Flags: review?(dflanagan) → review+
I fixed the transparency issue.

Let me see if I can explain the canplaythrough event - this should also explain the behaviour on pause.

I saw the spinner as an indicator saying that the video does not have enough data loaded to play through the whole video.

Consider if playback stops for more data and the waiting event is fired. This will cause the spinner to start. Here are the possibilities for the state to change:
    play is pushed: we're still waiting on network to data to play all the way
                    through, but we should still hide the spinner so they can
                    watch the video.
    playing is received: the video auto-resumed, the video should be able to
                         be played all the way through, remove the spinner
    they push the pause button: in this case, we won't get a play or playing
I hit tab then enter, sorry. Anyways:

they push the pause button: in this case, we won't get a play or playing
                            event, but we should still hide the spinner
                            when the video is ready, so we remove the
                            spinner on the canplaythrough event
(In reply to Leo from comment #31)
> Created attachment 758962 [details]
> Pointer to pull request
> 
> Hi David,
> 
> Please review the patch.
> This implementation is almost same as the one used for embedded video player
> (in gecko toolkit).
> 

I didn't see this comment before giving r- on your patch. It looks like you're talking about the code in mozilla-central/toolkit/widgets/videocontrols.xml

That makes me more favorably inclined toward your patch, but I still don't like the complexity. I also note that the code in videocontrols.xml is being used to decide whether to show and hide the full set of controls, not just the spinner. So I think that (uncommented) code is doing a lot more than we need to do here.

> I will test eshapiro's patch with the patch appied for bug 870564 and update
> you the results.

Thanks for testing it. Since today is the QC2 deadline, I'm likely to land eshapiro's patch because it is the theoretically correct way to fix this bug and is at least a partial fix. We can file a followup bug to add more code if we need it.
Of the two patches attached to this bug, eshapiro's is the theoretically correct one. Leo's may perform better in practice in Leo's testing, but I don't understand the code.  Because this bug is a QE2 bug, I want to at least take a step in the right direction. So I'm landing the simple, theoretically correct patch

Landed on master: https://github.com/mozilla-b2g/gaia/commit/1dbb4dbaa6c3022ff7bb30f891af28418e995eea

I've had a very hard time testing this, however, so requesting qawanted to help verify this fix.  We may need to open a new bug for further tweaking.
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: qawanted
Resolution: --- → FIXED
I'll leave uplift to jhford on this one.
Keywords: qawantedverifyme
Uplifted 1dbb4dbaa6c3022ff7bb30f891af28418e995eea to:
v1-train: 927b4ae2c159c701678ac0a878563439f052df08
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap?(cschmoeckel)
Added Browser Suite Test Case #8907 - Progress animation spinner is displayed when a streaming video is buffering
Flags: in-moztrap?(cschmoeckel) → in-moztrap+
Verified fixed on Leo Build ID: 20130723070209 on a 2G network, unable to check 3G because it's not available in our area. 

Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/68fb0a2e0114
Gaia: ffe25bfdf0e71c820ca710cb61fb8306564a8f4e
Platform Version: 18.1
RIL Version: 01.01.00.019.171
Verifying the issue is no longer reproduce on v1.1 device
The icon clearly shows when a video is buffering

Device: (Buri v1.1 Mozilla RIL)
BuildID: 20131015041203
Gaia: 680f3b86b1e4ff1411ece6ba397b8b0e56b4b31c
Gecko: 4bfd6a51cd05
Version: 18.0
You need to log in before you can comment on or make changes to this bug.