|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
STR. 1. Go to https://www.youtube.com/watch?v=cxeQmF4sUJY 2. Seek to arbitrary position Expected. 3. Sound icon shouldn't disappear during seeking Actual. 3. Sound icon disappear during seeking
In this case, this process would trigger MediaElement::Pause() before the MediaElement::Seek(), so the problem happen. However, this behavior isn't equal to every MediaElement. I tried to seek another audio element , and I found that the MediaElement::Seek() was triggered before MediaElement::Seek(). Here are two things we need to get them more clear, First, do we need to call Pause() when seeking the video? Second, what's the correct behavior when seeking the MediaElement?  http://people.mozilla.org/~alwu/Test.html
1) Your Step 2 isn't precise enough. Does "seek" means "click on progressbar" or "hover mouse over progressbar, hold left mouse button, then move mouse pointer within progressbar, then release left mouse button"? Or maybe "Hold Right arrow on the keyboard"? Actually, in all 3 cases producing sound is undesired option and therefore 2) The "Actual" result in comments 0-1 is completely expected, since nothing is causing sound. It's not clear why you're calling a problem the sound indicator being hidden when there's no sound See also bug 1225000
Hi, Arni, In this issue, the case I want to discuss here is "click on progressbar". The sound icon should only disappear when the second case you mentioned happen (hover mouse over progressbar...) Another interesting point is the third case (Hold Right arrow on the keyboard) doesn't let the sound icon disappear, and it's exactly the ideal behavior in my mind. However, IMMO, the sound icon shouldn't disappear in rest of these cases. Seeking is a transition behavior, it happens during very short period, we should keep the sound icon state as same as the state before seeking. If you think the sound icon need to disappear during this very short period, that means the users would see the icon blinking when they watch the video with a lots of short silent. In addition, the UX suggest is in . It said that we only need to handle the silence longer than 10 seconds. The bug 1225000 should also follow the 10-seconds rule.  https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c18
Hi, Philipp, Sorry to bother you, need your help again! Could you help me verify whether the sound icon should be displayed or not in the following situations? * pre-condition : Users are watching an audible video, and then they want to seek video * Here are three different kinds of seeking. (1) Click on the specific position on the progress-bar, they want the video start playing from that position. (2) Hover mouse over progress-bar, hold left mouse button, then move mouse pointer within progress-bar, then user can find the position they want from the preview image, release left mouse button until they decide where the video should start playing from. (3) Hold left/right arrow on the keyboard, to decrease/increase a fixed-interval for position. Very appreciate :)
I see now, you only want to reduce blinking in this bug. However, messing with timeouts doesn't look like a good idea: If in scenario (2) I move mouse about 10.1-10.5 seconds and then release left mouse button, then I will still see blinking "visible -> invisible -> visible" Also, this was already requested in bug 1232357. That's really ~5 lines of CSS to make a transition. Anyway, here's a few notes about comments 3-4: (1) in comment 4 is subset of (2), so this somehow reduces the number of cases... Scenario (3) actually doesn't hide the sound indicator, yes. That's because it doesn't stop the video! And sometimes I hear some short sounds during seeking. Video/audio element can't tell the difference between "press" and "hold", and that sounds like a separate bug (enhancement?) to me.
I like the idea of the bug1232357, if we can do this kinds of fading animation that we wouldn't worry about the icon blinking. If so, I think this issue should be solved in the front-end side, we would still dispatch the "audio-playback-false" event when the audio pausing or no any audible sound. However, the front-end side need to handle the logic about icon disappearing (or fading).
(In reply to Alastor Wu [:alwu] from comment #4) > Hi, Philipp, > Sorry to bother you, need your help again! > Could you help me verify whether the sound icon should be displayed or not > in the following situations? > > * pre-condition : Users are watching an audible video, and then they want to > seek video * > > Here are three different kinds of seeking. > (1) Click on the specific position on the progress-bar, they want the video > start playing from that > position. > (2) Hover mouse over progress-bar, hold left mouse button, then move mouse > pointer within progress-bar, > then user can find the position they want from the preview image, > release left mouse button until they > decide where the video should start playing from. > (3) Hold left/right arrow on the keyboard, to decrease/increase a > fixed-interval for position. > > Very appreciate :) Hm, if I understand you correctly, all of the scenarios depend upon whether or not sound is played during seeking. If the page keeps playing audio while seeking: keep showing the indicator. If not: wait for the timeout and if it still isn't playing audio after that: hide it. I also like the idea of fading the indicator in and out. That would make things a lot smoother. It might not be possible though, because the indicator is changing the size of the tab label, and I don't think we can animate that easily.
I would modify our native video-control, let the sound icon keeps showing during the seeking with clicking. So the STR is like following, STR. 1. Go to http://people.mozilla.org/~alwu/Test.html 2. Play video 3. Use mouse to click on the specific position on the progress-bar, and then release mouse Expected. 4. Sound icon shouldn't disappear during seeking Actual. 4. Sound icon disappear during seeking
For youtube, they have their own video-control-interface, so we can't control it.
Created attachment 8720215 [details] Bug 1239372 - only pause video during draging. Review commit: https://reviewboard.mozilla.org/r/35231/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35231/
Hi, Jared, Sorry to bother you, could you help me review this patch? Very appreciate :) --- The main idea of this patch is to prevent the tab sound indicator flickering when users use clicking to seek the video. The reproduce steps is in the comment8. I also list the possible seeking behaviors in comment4, we only need to call video.pause in the case (2), when user is continually dragging the progress-bar and doens't trigger mouse-up.
Hi Alastor, thanks for the patch. Fixing it in the video controls will only make the experience better for HTML5 media that uses our default controls, but not sites like YouTube that provide their own controls. A better fix would be bug 1232357 which will work everywhere and should cover the case of seeking within a video.
Ok, thanks :)
Hi, Jared, I think we still need that because on Fennec we use the default control interface, the media control  would be flashing if we don't fix this issue. The bug 1232357 only affects on the desktop. How do you think? Thanks!
FYI, Youtube uses our default control interface on Fennec.
I didn't think of that before. I'm a bit worried about not pausing during dragging and what other types of bugs we may introduce due to the amount of state that we store in the videocontrols binding. I think a more robust fix, and one that would also fix bug 1232357 would be to change the lower level code that announces audio has stopped. That code should have the 3 second delay built in to it.
Hi, Jaren, Sorry for the late reply. Could you help me explain the potential side affect if we do seeking before pausing? IMMO, the platform audio notification should be reflected the audio playing state precisely. If we want to do some delay, it might be implemented in front-end code for each different platform. Eg. The media control interface on Fennec is also depend on the platform audio notification to decide when it should be show/hide. And I don't want to delay the hiding time for that. Thanks!
Comment on attachment 8720215 [details] Bug 1239372 - only pause video during draging. https://reviewboard.mozilla.org/r/35231/#review56302 Okay, let's go with this for now and see what comes out of it. The code looks okay and not too different than what we were doing before besides the behavioral change.
(In reply to Alastor Wu [:alwu] from comment #18) > Hi, Jaren, > Sorry for the late reply. > Could you help me explain the potential side affect if we do seeking before > pausing? We have had bugs before about the videocontrols making some assumptions about the internal state of the video due to some other state being cached by the videocontrols. I'm not sure if this will cause any of those but we can try this out to see. It looks sound to me. > IMMO, the platform audio notification should be reflected the audio playing > state precisely. If we want to do some delay, it might be implemented in > front-end code for each different platform. > > Eg. The media control interface on Fennec is also depend on the platform > audio notification to decide when it should be show/hide. And I don't want > to delay the hiding time for that. Yes, you are right. We shouldn't lie about when the audio stops. The delay should remain a front-end feature.
Comment on attachment 8720215 [details] Bug 1239372 - only pause video during draging. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35231/diff/1-2/
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/8cef62921024 only pause video during draging. r=jaws
I have reproduced this bug with Firefox Nightly 46.0a1 (Build ID:20160113030208) on Windows 8.1, 64-bit. Verified as fixed with Firefox Nightly 50.0a1 (Build ID: 20160713030216) Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Verified as fixed on Firefox Aurora 50.0a2(2016-09-09) using Linux Mint 18.
Verified on FF 50.3beta [testday-20160930]
Based on comment 25, comment 26 and comment 28, I will set the corresponding flags.