Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
164032:
Disallow setting the rate to values that would cause busy loops (< 1/16x) or ... (Closed)
|
Created:
11 years, 4 months ago by kylep Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, Alpha Left Google, kylep, awong, darin (slow to review) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDisallow setting the rate to values that would cause busy loops (< 1/16x) or rendering issues (> 16x)
BUG=18362
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23037
Patch Set 1 #
Total comments: 2
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #Messages
Total messages: 18 (0 generated)
Frank and I decided on these values. Due to my other patch for muting audio on bad rates, these limits on video playback should not cause any issues. Please review.
Just wondering if we should clamp instead? http://codereview.chromium.org/164032/diff/1/2 File webkit/glue/webmediaplayer_impl.cc (right): http://codereview.chromium.org/164032/diff/1/2#newcode255 Line 255: // become unresponsive. I wonder if ignoring the user is best, or we should clamp to our min/max? Most UI's will expose reasonable values I expect, or some sort of continuous range, so its likely not a problem in practice. But each browser will be different with no great way to publish what your min/max are. So if a site had fast forward button that does 32x, it would fail on each browser in a different way. If we did our max (16x), that might make them happier?
magic numbers :( ... can you add a comment explaining how you arrived at these values?-darin On Wed, Aug 5, 2009 at 5:24 PM, <kylep@chromium.org> wrote: > Reviewers: scherkus, Alpha, awong, > > Message: > Frank and I decided on these values. Due to my other patch for muting > audio on bad rates, these limits on video playback should not cause any > issues. Please review. > > Description: > Disallow setting the rate to values that would cause busy loops (< > 1/16x) or rendering issues (> 16x) > BUG=18362 > TEST=none > > Please review this at http://codereview.chromium.org/164032 > > SVN Base: svn://chrome-svn/chrome/trunk/src/ > > Affected files: > M webkit/glue/webmediaplayer_impl.cc > > > Index: webkit/glue/webmediaplayer_impl.cc > =================================================================== > --- webkit/glue/webmediaplayer_impl.cc (revision 22506) > +++ webkit/glue/webmediaplayer_impl.cc (working copy) > @@ -248,6 +248,14 @@ > void WebMediaPlayerImpl::setRate(float rate) { > DCHECK(MessageLoop::current() == main_loop_); > > + // Limit rates to reasonable values. > + // TODO(kylep): Revisit these. There is an implicit upper bound due to > + // decoding rate. Also, Vista has an issue rendering frames too quickly. > + // Lastly, setting the rate too low causes us to enter busy loops and > + // become unresponsive. > + if (rate < 0.0625f || rate > 16.0f) > + return; > + > playback_rate_ = rate; > if (!paused_) { > pipeline_->SetPlaybackRate(rate); > > >
also, what is the vista issue?-darin On Wed, Aug 5, 2009 at 7:53 PM, Darin Fisher <darin@chromium.org> wrote: > magic numbers :( ... can you add a comment explaining how you arrived at > these values?-darin > > > On Wed, Aug 5, 2009 at 5:24 PM, <kylep@chromium.org> wrote: > >> Reviewers: scherkus, Alpha, awong, >> >> Message: >> Frank and I decided on these values. Due to my other patch for muting >> audio on bad rates, these limits on video playback should not cause any >> issues. Please review. >> >> Description: >> Disallow setting the rate to values that would cause busy loops (< >> 1/16x) or rendering issues (> 16x) >> BUG=18362 >> TEST=none >> >> Please review this at http://codereview.chromium.org/164032 >> >> SVN Base: svn://chrome-svn/chrome/trunk/src/ >> >> Affected files: >> M webkit/glue/webmediaplayer_impl.cc >> >> >> Index: webkit/glue/webmediaplayer_impl.cc >> =================================================================== >> --- webkit/glue/webmediaplayer_impl.cc (revision 22506) >> +++ webkit/glue/webmediaplayer_impl.cc (working copy) >> @@ -248,6 +248,14 @@ >> void WebMediaPlayerImpl::setRate(float rate) { >> DCHECK(MessageLoop::current() == main_loop_); >> >> + // Limit rates to reasonable values. >> + // TODO(kylep): Revisit these. There is an implicit upper bound due to >> + // decoding rate. Also, Vista has an issue rendering frames too >> quickly. >> + // Lastly, setting the rate too low causes us to enter busy loops and >> + // become unresponsive. >> + if (rate < 0.0625f || rate > 16.0f) >> + return; >> + >> playback_rate_ = rate; >> if (!paused_) { >> pipeline_->SetPlaybackRate(rate); >> >> >> >
There are 2 Vista specific issues high speed graphics Vista has substantially lower performance than XP or Windows7. If you speed up a video too much, it can't keep up, and rendering stops updating.... except the time bar.If you go rediculously fast (ie 50000x) we get a 'snap'. We've only seen it on Vista. There had been an issue with high speed/performance in the 12x range. A buffer overrun. Vista can't play back fast enough and was causing a snap. We now just use up the data we have, which may not achieve the speed you ask for, but won't "snap". Even my low end Pentium4 with XP doesn't have this problem up to 32x. There is one issue with low speed, which may be Vista. A very slow speed. ie 0.00000001x, causes the machine to lock up a bit... seems like a busy loop. It get a bit unresponsive, although its not completely dead. Also our timers are not very accurate, which becomes evident at low speeds, and on Vista. Since these are risky and outside the norms, we think 1/16x to 16x is a safe and useful range. The issues dont seem to affect Windows7 or XP, making it lower priority. Most Vista users will have higher end PC's, and wont be trying to do 1080p at high or low 'rate'. On Wed, Aug 5, 2009 at 7:54 PM, Darin Fisher <darin@chromium.org> wrote: > also, what is the vista issue?-darin > > > On Wed, Aug 5, 2009 at 7:53 PM, Darin Fisher <darin@chromium.org> wrote: > >> magic numbers :( ... can you add a comment explaining how you arrived at >> these values?-darin >> >> >> On Wed, Aug 5, 2009 at 5:24 PM, <kylep@chromium.org> wrote: >> >>> Reviewers: scherkus, Alpha, awong, >>> >>> Message: >>> Frank and I decided on these values. Due to my other patch for muting >>> audio on bad rates, these limits on video playback should not cause any >>> issues. Please review. >>> >>> Description: >>> Disallow setting the rate to values that would cause busy loops (< >>> 1/16x) or rendering issues (> 16x) >>> BUG=18362 >>> TEST=none >>> >>> Please review this at http://codereview.chromium.org/164032 >>> >>> SVN Base: svn://chrome-svn/chrome/trunk/src/ >>> >>> Affected files: >>> M webkit/glue/webmediaplayer_impl.cc >>> >>> >>> Index: webkit/glue/webmediaplayer_impl.cc >>> =================================================================== >>> --- webkit/glue/webmediaplayer_impl.cc (revision 22506) >>> +++ webkit/glue/webmediaplayer_impl.cc (working copy) >>> @@ -248,6 +248,14 @@ >>> void WebMediaPlayerImpl::setRate(float rate) { >>> DCHECK(MessageLoop::current() == main_loop_); >>> >>> + // Limit rates to reasonable values. >>> + // TODO(kylep): Revisit these. There is an implicit upper bound due to >>> + // decoding rate. Also, Vista has an issue rendering frames too >>> quickly. >>> + // Lastly, setting the rate too low causes us to enter busy loops and >>> + // become unresponsive. >>> + if (rate < 0.0625f || rate > 16.0f) >>> + return; >>> + >>> playback_rate_ = rate; >>> if (!paused_) { >>> pipeline_->SetPlaybackRate(rate); >>> >>> >>> >> >
thanks for the explanation frank! now we need kyle to use it as a comment for the constants he should be adding ;)
I'm still confused. Chrome rate limits rendering based on calls to the Win32 API RedrawWindow. That restricts how fast the calls to WebFrame::Paint arrive. Are you saying that you sometimes "render" more often than WebFrame::Paint? -Darin On Wed, Aug 5, 2009 at 8:11 PM, Frank Barchard <fbarchard@google.com> wrote: > There are 2 Vista specific issues high speed graphics > > Vista has substantially lower performance than XP or Windows7. If you > speed up a video too much, it can't keep up, and rendering stops > updating.... except the time bar. If you go rediculously fast (ie 50000x) > we get a 'snap'. We've only seen it on Vista. > > There had been an issue with high speed/performance in the 12x range. A > buffer overrun. Vista can't play back fast enough and was causing a snap. > We now just use up the data we have, which may not achieve the speed you > ask for, but won't "snap". Even my low end Pentium4 with XP doesn't have > this problem up to 32x. > > There is one issue with low speed, which may be Vista. A very slow speed. > ie 0.00000001x, causes the machine to lock up a bit... seems like a busy > loop. It get a bit unresponsive, although its not completely dead. > > Also our timers are not very accurate, which becomes evident at low speeds, > and on Vista. > Since these are risky and outside the norms, we think 1/16x to 16x is a > safe and useful range. > The issues dont seem to affect Windows7 or XP, making it lower priority. > Most Vista users will have higher end PC's, and wont be trying to do 1080p > at high or low 'rate'. > > > On Wed, Aug 5, 2009 at 7:54 PM, Darin Fisher <darin@chromium.org> wrote: > >> also, what is the vista issue?-darin >> >> >> On Wed, Aug 5, 2009 at 7:53 PM, Darin Fisher <darin@chromium.org> wrote: >> >>> magic numbers :( ... can you add a comment explaining how you arrived at >>> these values?-darin >>> >>> >>> On Wed, Aug 5, 2009 at 5:24 PM, <kylep@chromium.org> wrote: >>> >>>> Reviewers: scherkus, Alpha, awong, >>>> >>>> Message: >>>> Frank and I decided on these values. Due to my other patch for muting >>>> audio on bad rates, these limits on video playback should not cause any >>>> issues. Please review. >>>> >>>> Description: >>>> Disallow setting the rate to values that would cause busy loops (< >>>> 1/16x) or rendering issues (> 16x) >>>> BUG=18362 >>>> TEST=none >>>> >>>> Please review this at http://codereview.chromium.org/164032 >>>> >>>> SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>> >>>> Affected files: >>>> M webkit/glue/webmediaplayer_impl.cc >>>> >>>> >>>> Index: webkit/glue/webmediaplayer_impl.cc >>>> =================================================================== >>>> --- webkit/glue/webmediaplayer_impl.cc (revision 22506) >>>> +++ webkit/glue/webmediaplayer_impl.cc (working copy) >>>> @@ -248,6 +248,14 @@ >>>> void WebMediaPlayerImpl::setRate(float rate) { >>>> DCHECK(MessageLoop::current() == main_loop_); >>>> >>>> + // Limit rates to reasonable values. >>>> + // TODO(kylep): Revisit these. There is an implicit upper bound due >>>> to >>>> + // decoding rate. Also, Vista has an issue rendering frames too >>>> quickly. >>>> + // Lastly, setting the rate too low causes us to enter busy loops and >>>> + // become unresponsive. >>>> + if (rate < 0.0625f || rate > 16.0f) >>>> + return; >>>> + >>>> playback_rate_ = rate; >>>> if (!paused_) { >>>> pipeline_->SetPlaybackRate(rate); >>>> >>>> >>>> >>> >> >
On Wed, Aug 5, 2009 at 8:11 PM, Frank Barchard <fbarchard@google.com> wrote: > Also our timers are not very accurate, which becomes evident at low speeds, > and on Vista. > Since these are risky and outside the norms, we think 1/16x to 16x is a > safe and useful range. > I can guarantee you that eventually people will want MUCH more than 16x. My PVR at home does 300x on the fastest fast-forward, which I like. Even 60x is useful. As for going slow, 1/30x is useful to do one fps for video. Perhaps since your problems happen at more extreme range points, you can use a wider range, like 1/128x - 512x ? This would be more future-proof. Also, I suggest clamping values outside the range, rather than ignoring them. PK Please review this at http://codereview.chromium.org/164032 >>> >>>
On Wed, Aug 5, 2009 at 8:46 PM, Peter Kasting <pkasting@chromium.org> wrote: > On Wed, Aug 5, 2009 at 8:11 PM, Frank Barchard <fbarchard@google.com>wrote: > >> Also our timers are not very accurate, which becomes evident at low >> speeds, and on Vista. >> Since these are risky and outside the norms, we think 1/16x to 16x is a >> safe and useful range. >> > > I can guarantee you that eventually people will want MUCH more than 16x. > My PVR at home does 300x on the fastest fast-forward, which I like. Even > 60x is useful. > Firstly, users will be very disappointed if we crash. The priority is to make <video> rock solid. At 16x, we're doing way better than Safari, Flash or Firefox can do. If they want more, and I know I do, then we can work on that. > > As for going slow, 1/30x is useful to do one fps for video. > Ya I know its a bit useful... but we have a bugs on low speed that aren't worth the risk. I use movie formats as slide shows. ie fps = 1/5 is 5 seconds per frame. Sometimes you want to scrutenize every frame. I'd love to see high speed cameras/video, especially for sports. And its somewhat becoming mainstream. I know a couple people with high speed, high def cameras. They end up showing us super slow motion at 30fps. But wouldnt it be cool if your favorite sports were high frame rate, and maybe your TV is 120 fps and looks pretty good, but when you play in slow motion, its still super smooth. Naysayers would say its a waste of bandwidth, but each doubling in framerate only increases bandwidth by sqrt(2), because the differences become smaller. I'd rather see high framerate 720p than 1080p or ultraHD. I made a sample video filmed at 1000 fps, and if you slow it to 1/40x you get it in the speed the filmmaker showed it in (25 fps). > > Perhaps since your problems happen at more extreme range points, you can > use a wider range, like 1/128x - 512x ? This would be more future-proof. > It would cause definate stability issues on Vista. We should fix those first. > > Also, I suggest clamping values outside the range, rather than ignoring > them. > Thats what I was thinking. It'll help the user know what the limit is, or not care too much. > > PK > > Please review this at http://codereview.chromium.org/164032 >>>> >>>>
On 2009/08/06 03:51:22, darin wrote: > I'm still confused. Chrome rate limits rendering based on calls to the > Win32 API RedrawWindow. That restricts how fast the calls to > WebFrame::Paint arrive. Are you saying that you sometimes "render" more > often than WebFrame::Paint? > -Darin Every time a frame is ready, an angel calls Repaint() :) I'm OOO and without vista machines, so I haven't had a chance to debug the issue. My hunch is it's related to our repaint-limiting code. Since we're notified that a frame is ready on a different thread, we have to post a task to the render thread to call repaint. Looking through the code it looks like that maximum was set to 50 pending tasks to call Repaint(). I'm not familiar with Repaint() and whether it no-ops if we're already painting, but it could be that on Vista having 50 repaint tasks causes glitchy drawing and/or visual "snaps". The way our code is set up that maximum shouldn't be more than ~1 (if you miss your deadline to grab the frame + paint it is dropped to the floor -- extra Repaint()s will not save you). Kyle/Frank, care to set kMaxOutstandingRepaints to 1 and see what happens?
okay, that blows one theory, so I'm confused too. Here's what I've observed:Vista64 Chrome video is substantially slower than Windows XP or Windows 7. We suspect GDI is very slow, being software rendered.Low resolutions are okay, but if we take a 1080p video and... .. increase rate about 4x, we start to stuggle, dropping frames. XP has no problem with this... increase rate about 10x, the frame stops updating completely. The time bar keeps animating. .. beyond 10x degrades further, including "snaps" Audio also has problems with 96 Khz and beyond. XP and Windows7 can do 192 Khz okay. This doesn't happen in --single-process, so it may also be an IPC/threading issue. For a tiny video, 1000 fps plays okay. So its not purely a paint/message rate issue. On Wed, Aug 5, 2009 at 8:46 PM, Darin Fisher <darin@chromium.org> wrote: > I'm still confused. Chrome rate limits rendering based on calls to the > Win32 API RedrawWindow. That restricts how fast the calls to > WebFrame::Paint arrive. Are you saying that you sometimes "render" more > often than WebFrame::Paint? > -Darin > > > On Wed, Aug 5, 2009 at 8:11 PM, Frank Barchard <fbarchard@google.com>wrote: > >> There are 2 Vista specific issues high speed graphics >> >> Vista has substantially lower performance than XP or Windows7. If you >> speed up a video too much, it can't keep up, and rendering stops >> updating.... except the time bar. If you go rediculously fast (ie 50000x) >> we get a 'snap'. We've only seen it on Vista. >> >> There had been an issue with high speed/performance in the 12x range. A >> buffer overrun. Vista can't play back fast enough and was causing a snap. >> We now just use up the data we have, which may not achieve the speed you >> ask for, but won't "snap". Even my low end Pentium4 with XP doesn't have >> this problem up to 32x. >> >> There is one issue with low speed, which may be Vista. A very slow speed. >> ie 0.00000001x, causes the machine to lock up a bit... seems like a busy >> loop. It get a bit unresponsive, although its not completely dead. >> >> Also our timers are not very accurate, which becomes evident at low >> speeds, and on Vista. >> Since these are risky and outside the norms, we think 1/16x to 16x is a >> safe and useful range. >> The issues dont seem to affect Windows7 or XP, making it lower priority. >> Most Vista users will have higher end PC's, and wont be trying to do 1080p >> at high or low 'rate'. >> >> >> On Wed, Aug 5, 2009 at 7:54 PM, Darin Fisher <darin@chromium.org> wrote: >> >>> also, what is the vista issue?-darin >>> >>> >>> On Wed, Aug 5, 2009 at 7:53 PM, Darin Fisher <darin@chromium.org> wrote: >>> >>>> magic numbers :( ... can you add a comment explaining how you arrived at >>>> these values?-darin >>>> >>>> >>>> On Wed, Aug 5, 2009 at 5:24 PM, <kylep@chromium.org> wrote: >>>> >>>>> Reviewers: scherkus, Alpha, awong, >>>>> >>>>> Message: >>>>> Frank and I decided on these values. Due to my other patch for muting >>>>> audio on bad rates, these limits on video playback should not cause any >>>>> issues. Please review. >>>>> >>>>> Description: >>>>> Disallow setting the rate to values that would cause busy loops (< >>>>> 1/16x) or rendering issues (> 16x) >>>>> BUG=18362 >>>>> TEST=none >>>>> >>>>> Please review this at http://codereview.chromium.org/164032 >>>>> >>>>> SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>> >>>>> Affected files: >>>>> M webkit/glue/webmediaplayer_impl.cc >>>>> >>>>> >>>>> Index: webkit/glue/webmediaplayer_impl.cc >>>>> =================================================================== >>>>> --- webkit/glue/webmediaplayer_impl.cc (revision 22506) >>>>> +++ webkit/glue/webmediaplayer_impl.cc (working copy) >>>>> @@ -248,6 +248,14 @@ >>>>> void WebMediaPlayerImpl::setRate(float rate) { >>>>> DCHECK(MessageLoop::current() == main_loop_); >>>>> >>>>> + // Limit rates to reasonable values. >>>>> + // TODO(kylep): Revisit these. There is an implicit upper bound due >>>>> to >>>>> + // decoding rate. Also, Vista has an issue rendering frames too >>>>> quickly. >>>>> + // Lastly, setting the rate too low causes us to enter busy loops >>>>> and >>>>> + // become unresponsive. >>>>> + if (rate < 0.0625f || rate > 16.0f) >>>>> + return; >>>>> + >>>>> playback_rate_ = rate; >>>>> if (!paused_) { >>>>> pipeline_->SetPlaybackRate(rate); >>>>> >>>>> >>>>> >>>> >>> >> >
I'll tried 2, so that we can have one for the UI and one for the frame. src\webkit\glue\webmediaplayer_impl.cc and sometimes 1 can cause deadlock type issues. No improvement. At 2x on red just the timebar updates. But I noticed that if I change the window size as the movie plays, the frames display reasonably nicely. Audio is fine. CPU usage is 81%, and if I do the resizing trick, its 100%, but the machine is fine. Also at the end, when music finishes a bit before the video, the video plays fine. Theory - we are constantly a little behind due to a latency...potentially its the 30ms audio latency I mentioned. The video renderer thinks we're behind and tries to skip frames to catch up. While writting the email, I let the video sit at the end for about 2 minutes, and then got a "snap". I'll assign that to Alpha, who plans to look at end of file issues. crbug.com/18607 On Wed, Aug 5, 2009 at 9:09 PM, <scherkus@chromium.org> wrote: > On 2009/08/06 03:51:22, darin wrote: > >> I'm still confused. Chrome rate limits rendering based on calls to >> > the > >> Win32 API RedrawWindow. That restricts how fast the calls to >> WebFrame::Paint arrive. Are you saying that you sometimes "render" >> > more > >> often than WebFrame::Paint? >> -Darin >> > > Every time a frame is ready, an angel calls Repaint() :) > > I'm OOO and without vista machines, so I haven't had a chance to debug > the issue. My hunch is it's related to our repaint-limiting code. > Since we're notified that a frame is ready on a different thread, we > have to post a task to the render thread to call repaint. Looking > through the code it looks like that maximum was set to 50 pending tasks > to call Repaint(). > > I'm not familiar with Repaint() and whether it no-ops if we're already > painting, but it could be that on Vista having 50 repaint tasks causes > glitchy drawing and/or visual "snaps". > > The way our code is set up that maximum shouldn't be more than ~1 (if > you miss your deadline to grab the frame + paint it is dropped to the > floor -- extra Repaint()s will not save you). > > Kyle/Frank, care to set kMaxOutstandingRepaints to 1 and see what > happens? > > > http://codereview.chromium.org/164032 >
Modified Theory - Kyles code adds latency to audio. On XP audio is lower latency and we get away with it. Vista adds a large constant to this. To test this, I'll make a 2x faster version of red, without the pitch shifting code. It'll take a few hours, so we'll test that tomorrow. Kyle, do you know how many milliseconds your pitch shifter is? If I'm right, the solution might be to update the audio timer in a way that compensates for high latency audio. Or just allow more slop in framerate without dropping frames. On Wed, Aug 5, 2009 at 10:26 PM, Frank Barchard <fbarchard@google.com>wrote: > I'll tried 2, so that we can have one for the UI and one for the frame. > src\webkit\glue\webmediaplayer_impl.cc > and sometimes 1 can cause deadlock type issues. > > No improvement. > > At 2x on red just the timebar updates. > But I noticed that if I change the window size as the movie plays, the > frames display reasonably nicely. Audio is fine. > CPU usage is 81%, and if I do the resizing trick, its 100%, but the machine > is fine. > Also at the end, when music finishes a bit before the video, the video > plays fine. > > Theory - we are constantly a little behind due to a latency...potentially > its the 30ms audio latency I mentioned. The video renderer thinks we're > behind and tries to skip frames to catch up. > > While writting the email, I let the video sit at the end for about 2 > minutes, and then got a "snap". I'll assign that to Alpha, who plans to > look at end of file issues. > crbug.com/18607 > > On Wed, Aug 5, 2009 at 9:09 PM, <scherkus@chromium.org> wrote: > >> On 2009/08/06 03:51:22, darin wrote: >> >>> I'm still confused. Chrome rate limits rendering based on calls to >>> >> the >> >>> Win32 API RedrawWindow. That restricts how fast the calls to >>> WebFrame::Paint arrive. Are you saying that you sometimes "render" >>> >> more >> >>> often than WebFrame::Paint? >>> -Darin >>> >> >> Every time a frame is ready, an angel calls Repaint() :) >> >> I'm OOO and without vista machines, so I haven't had a chance to debug >> the issue. My hunch is it's related to our repaint-limiting code. >> Since we're notified that a frame is ready on a different thread, we >> have to post a task to the render thread to call repaint. Looking >> through the code it looks like that maximum was set to 50 pending tasks >> to call Repaint(). >> >> I'm not familiar with Repaint() and whether it no-ops if we're already >> painting, but it could be that on Vista having 50 repaint tasks causes >> glitchy drawing and/or visual "snaps". >> >> The way our code is set up that maximum shouldn't be more than ~1 (if >> you miss your deadline to grab the frame + paint it is dropped to the >> floor -- extra Repaint()s will not save you). >> >> Kyle/Frank, care to set kMaxOutstandingRepaints to 1 and see what >> happens? >> >> >> http://codereview.chromium.org/164032 >> > >
I paraphrased Frank's explanation above the constants. Are they in the correct place for this file? http://codereview.chromium.org/164032/diff/1/2 File webkit/glue/webmediaplayer_impl.cc (right): http://codereview.chromium.org/164032/diff/1/2#newcode255 Line 255: // become unresponsive. On 2009/08/06 02:45:23, fbarchard wrote: > I wonder if ignoring the user is best, or we should clamp to our min/max? > Most UI's will expose reasonable values I expect, or some sort of continuous > range, so its likely not a problem in practice. > But each browser will be different with no great way to publish what your > min/max are. So if a site had fast forward button that does 32x, it would fail > on each browser in a different way. If we did our max (16x), that might make > them happier? Done.
Fixed so negative values are ignored instead of clamping to 1/16x. Also changed to allow the user to pause by setting the rate to 0. Removed a now-unnecessary check in pipeline_impl.cc for rate < 0.
http://codereview.chromium.org/164032/diff/21/1009 File media/base/pipeline_impl.cc (left): http://codereview.chromium.org/164032/diff/21/1009#oldcode171 Line 171: if (playback_rate < 0.0f) { this might fail unit tests I still think we want this check, even if redundant (webmediaplayer_impl is not the only client) http://codereview.chromium.org/164032/diff/21/1008 File webkit/glue/webmediaplayer_impl.cc (right): http://codereview.chromium.org/164032/diff/21/1008#newcode272 Line 272: if (rate < 0) these zeros may need to be 0.0f ... try server will let you know
http://codereview.chromium.org/164032/diff/21/1009 File media/base/pipeline_impl.cc (left): http://codereview.chromium.org/164032/diff/21/1009#oldcode171 Line 171: if (playback_rate < 0.0f) { On 2009/08/10 18:25:51, scherkus wrote: > this might fail unit tests > > I still think we want this check, even if redundant (webmediaplayer_impl is not > the only client) Ok. Change removed. http://codereview.chromium.org/164032/diff/21/1008 File webkit/glue/webmediaplayer_impl.cc (right): http://codereview.chromium.org/164032/diff/21/1008#newcode272 Line 272: if (rate < 0) On 2009/08/10 18:25:51, scherkus wrote: > these zeros may need to be 0.0f ... try server will let you know Passed the tests, but I changed them to 0.0f anyway.
LGTM |
