Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
654203005:
Skip initial seek to 0 on fling (Closed)
DescriptionSkip initial seek to 0 on fling
The media player now won't seek after load when there is no actual
pending seek.
BUG=420645
Committed: https://crrev.com/43dd925535ac16c365508b6e727c4b6a5a0166fd
Cr-Commit-Position: refs/heads/master@{#300452}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Replaced the magic constant/invalid value with a boolean flag #
Total comments: 8
Patch Set 3 : Reset variables after seek, remove useless comment #Patch Set 4 : Formatting #
Messages
Total messages: 16 (3 generated)
dgn@chromium.org changed reviewers: + aberent@chromium.org, avayvod@chromium.org
dgn@chromium.org changed reviewers: + qinmin@chromium.org
qinmin@ please review as owner. whywhat@ and aberent@ will be doing the full review.
https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... media/base/android/media_player_bridge.cc:20: Why the extra blank line? https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... media/base/android/media_player_bridge.h:145: base::TimeDelta pending_seek_ = base::TimeDelta::FromMilliseconds(-1); Better in initializer of constructor. The coding standard is unclear on this, but other members of this class are initialised in the constructor so for consistency initialize all the variables here. Also, the magic number is nasty. Better to add a new boolean member to the class, and use that to decide whether to do a seek.
https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... media/base/android/media_player_bridge.cc:20: On 2014/10/20 14:55:55, aberent wrote: > Why the extra blank line? Done. https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... File media/base/android/media_player_bridge.h (right): https://codereview.chromium.org/654203005/diff/1/media/base/android/media_pla... media/base/android/media_player_bridge.h:145: base::TimeDelta pending_seek_ = base::TimeDelta::FromMilliseconds(-1); On 2014/10/20 14:55:55, aberent wrote: > Better in initializer of constructor. The coding standard is unclear on this, > but other members of this class are initialised in the constructor so for > consistency initialize all the variables here. > > Also, the magic number is nasty. Better to add a new boolean member to the > class, and use that to decide whether to do a seek. Done.
lgtm
https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:425: // if (pending_seek_.InMilliseconds() >= 0) { cannot we just change this line to > 0 and it will have the same effect? When mediaplayer becomes prepared, the position is always 0. So if we don't do any seek when pending_seek_ is not changed, it will have the same effect.
On 2014/10/20 17:16:19, qinmin wrote: > https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... > File media/base/android/media_player_bridge.cc (right): > > https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... > media/base/android/media_player_bridge.cc:425: // if > (pending_seek_.InMilliseconds() >= 0) { > cannot we just change this line to > 0 and it will have the same effect? > > When mediaplayer becomes prepared, the position is always 0. So if we don't do > any seek when pending_seek_ is not changed, it will have the same effect. I wrote ">=" because I considered that there might be some cases where people would actually want the video to start over from 0. Then we would want to perform a seek to 0 on prepare.
lgtm https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:425: // if (pending_seek_.InMilliseconds() >= 0) { On 2014/10/20 17:16:19, qinmin wrote: > cannot we just change this line to > 0 and it will have the same effect? > > When mediaplayer becomes prepared, the position is always 0. So if we don't do > any seek when pending_seek_ is not changed, it will have the same effect. Some websites actually do call video.seekTo(0) before calling video.play() so we should distinguish the actual seek to 0 from the default value. https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:427: PendingSeekInternal(pending_seek_); should we reset pending_seek_ to 0 and should_seek_on_prepare_ to false here? https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:428: } nit: no need for curly braces here
lgtm https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:425: // if (pending_seek_.InMilliseconds() >= 0) { remove this comments, it is not helpful for understanding the logic On 2014/10/20 18:41:35, whywhat wrote: > On 2014/10/20 17:16:19, qinmin wrote: > > cannot we just change this line to > 0 and it will have the same effect? > > > > When mediaplayer becomes prepared, the position is always 0. So if we don't do > > any seek when pending_seek_ is not changed, it will have the same effect. > > Some websites actually do call video.seekTo(0) before calling video.play() so we > should distinguish the actual seek to 0 from the default value.
https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:425: // if (pending_seek_.InMilliseconds() >= 0) { On 2014/10/20 21:44:52, qinmin wrote: > remove this comments, it is not helpful for understanding the logic > > On 2014/10/20 18:41:35, whywhat wrote: > > On 2014/10/20 17:16:19, qinmin wrote: > > > cannot we just change this line to > 0 and it will have the same effect? > > > > > > When mediaplayer becomes prepared, the position is always 0. So if we don't > do > > > any seek when pending_seek_ is not changed, it will have the same effect. > > > > Some websites actually do call video.seekTo(0) before calling video.play() so > we > > should distinguish the actual seek to 0 from the default value. > Done. https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:427: PendingSeekInternal(pending_seek_); On 2014/10/20 18:41:35, whywhat wrote: > should we reset pending_seek_ to 0 and should_seek_on_prepare_ to false here? Done. https://codereview.chromium.org/654203005/diff/20001/media/base/android/media... media/base/android/media_player_bridge.cc:428: } On 2014/10/20 18:41:35, whywhat wrote: > nit: no need for curly braces here They are needed now.
The CQ bit was checked by dgn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/654203005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/43dd925535ac16c365508b6e727c4b6a5a0166fd Cr-Commit-Position: refs/heads/master@{#300452} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
