Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22605013:
Fix HLS playback on android 4.3 (Closed)
|
Created:
7 years, 4 months ago by qinmin Modified:
7 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix HLS playback on android 4.3
For HLS, seekTo() can cause media player to stuck in a error state.
Since the returned duration < 0 for HLS, we can ignore all seek positions that are negative
BUG=265460
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216786
Patch Set 1 #
Total comments: 6
Patch Set 2 : nits #Messages
Total messages: 10 (0 generated)
PTAL
https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: Newbie question: in what situation could these conditions be true? Shouldn't the player/ui who issues the seek request check for these conditions? In that case, we only need to DCHECK here.
https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: For HLS, the duration is -1 as it is a live stream. After media player is prepared, we always call seek() to the last seek position. The seek position is there because user may switch tabs, or tap on the seek bar when player is not prepared. By default, seek(0) is called. For 4.2, seek will not cause any problems for live stream. For 4.3, it will stuck the player in an error state. On 2013/08/09 17:03:19, xhwang wrote: > Newbie question: in what situation could these conditions be true? Shouldn't the > player/ui who issues the seek request check for these conditions? In that case, > we only need to DCHECK here.
lgtm % one comment https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: On 2013/08/09 17:11:42, qinmin wrote: > For HLS, the duration is -1 as it is a live stream. > After media player is prepared, we always call seek() to the last seek position. > The seek position is there because user may switch tabs, or tap on the seek bar > when player is not prepared. By default, seek(0) is called. > For 4.2, seek will not cause any problems for live stream. > For 4.3, it will stuck the player in an error state. > > On 2013/08/09 17:03:19, xhwang wrote: > > Newbie question: in what situation could these conditions be true? Shouldn't > the > > player/ui who issues the seek request check for these conditions? In that > case, > > we only need to DCHECK here. > Thanks for the explanation. So if time < base::TimeDelta(), can we DCHECK_EQ(time, -1)?
lgtm % nit https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: On 2013/08/09 17:15:54, xhwang wrote: > On 2013/08/09 17:11:42, qinmin wrote: > > For HLS, the duration is -1 as it is a live stream. > > After media player is prepared, we always call seek() to the last seek > position. > > The seek position is there because user may switch tabs, or tap on the seek > bar > > when player is not prepared. By default, seek(0) is called. > > For 4.2, seek will not cause any problems for live stream. > > For 4.3, it will stuck the player in an error state. > > > > On 2013/08/09 17:03:19, xhwang wrote: > > > Newbie question: in what situation could these conditions be true? Shouldn't > > the > > > player/ui who issues the seek request check for these conditions? In that > > case, > > > we only need to DCHECK here. > > > > Thanks for the explanation. So if time < base::TimeDelta(), can we > DCHECK_EQ(time, -1)? Also thanks for asking and answering. Another newbie question: Why are we using -1 literal rather than a well-defined constant for this in code? Like kInfiniteDuration and kNoTimeStaamp, something like kSeekingInHLS??
On 2013/08/09 18:13:19, wolenetz wrote: > lgtm % nit > > https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... > File media/base/android/media_player_bridge.cc (right): > > https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... > media/base/android/media_player_bridge.cc:400: > On 2013/08/09 17:15:54, xhwang wrote: > > On 2013/08/09 17:11:42, qinmin wrote: > > > For HLS, the duration is -1 as it is a live stream. > > > After media player is prepared, we always call seek() to the last seek > > position. > > > The seek position is there because user may switch tabs, or tap on the seek > > bar > > > when player is not prepared. By default, seek(0) is called. > > > For 4.2, seek will not cause any problems for live stream. > > > For 4.3, it will stuck the player in an error state. > > > > > > On 2013/08/09 17:03:19, xhwang wrote: > > > > Newbie question: in what situation could these conditions be true? > Shouldn't > > > the > > > > player/ui who issues the seek request check for these conditions? In that > > > case, > > > > we only need to DCHECK here. > > > > > > > Thanks for the explanation. So if time < base::TimeDelta(), can we > > DCHECK_EQ(time, -1)? > > Also thanks for asking and answering. Another newbie question: Why are we using > -1 literal rather than a well-defined constant for this in code? Like > kInfiniteDuration and kNoTimeStaamp, something like kSeekingInHLS?? Also, I'm not sure if this CL is uploaded correctly because rietveld gives chunk mismatch error when I try the 'View' link in PS1: https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play...
On 2013/08/09 18:16:46, wolenetz wrote: > On 2013/08/09 18:13:19, wolenetz wrote: > > lgtm % nit > > > > > https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... > > File media/base/android/media_player_bridge.cc (right): > > > > > https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... > > media/base/android/media_player_bridge.cc:400: > > On 2013/08/09 17:15:54, xhwang wrote: > > > On 2013/08/09 17:11:42, qinmin wrote: > > > > For HLS, the duration is -1 as it is a live stream. > > > > After media player is prepared, we always call seek() to the last seek > > > position. > > > > The seek position is there because user may switch tabs, or tap on the > seek > > > bar > > > > when player is not prepared. By default, seek(0) is called. > > > > For 4.2, seek will not cause any problems for live stream. > > > > For 4.3, it will stuck the player in an error state. > > > > > > > > On 2013/08/09 17:03:19, xhwang wrote: > > > > > Newbie question: in what situation could these conditions be true? > > Shouldn't > > > > the > > > > > player/ui who issues the seek request check for these conditions? In > that > > > > case, > > > > > we only need to DCHECK here. > > > > > > > > > > Thanks for the explanation. So if time < base::TimeDelta(), can we > > > DCHECK_EQ(time, -1)? > > > > Also thanks for asking and answering. Another newbie question: Why are we > using > > -1 literal rather than a well-defined constant for this in code? Like > > kInfiniteDuration and kNoTimeStaamp, something like kSeekingInHLS?? > > Also, I'm not sure if this CL is uploaded correctly because rietveld gives chunk > mismatch error when I try the 'View' link in PS1: > https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... This is probably a site issue, I have problem reviewing CLs since yesterday afternoon
https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: Done. On 2013/08/09 17:15:54, xhwang wrote: > On 2013/08/09 17:11:42, qinmin wrote: > > For HLS, the duration is -1 as it is a live stream. > > After media player is prepared, we always call seek() to the last seek > position. > > The seek position is there because user may switch tabs, or tap on the seek > bar > > when player is not prepared. By default, seek(0) is called. > > For 4.2, seek will not cause any problems for live stream. > > For 4.3, it will stuck the player in an error state. > > > > On 2013/08/09 17:03:19, xhwang wrote: > > > Newbie question: in what situation could these conditions be true? Shouldn't > > the > > > player/ui who issues the seek request check for these conditions? In that > > case, > > > we only need to DCHECK here. > > > > Thanks for the explanation. So if time < base::TimeDelta(), can we > DCHECK_EQ(time, -1)? https://codereview.chromium.org/22605013/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:400: -1 is the return value from android. Using kInfiniteDuration() has a side effect. If we return kInfiniteDuration() as the duration to WebMediaPlayerImpl, that's actually a valid duration for media elements. So user can toggle the media controller, which is not desirable On 2013/08/09 18:13:19, wolenetz wrote: > On 2013/08/09 17:15:54, xhwang wrote: > > On 2013/08/09 17:11:42, qinmin wrote: > > > For HLS, the duration is -1 as it is a live stream. > > > After media player is prepared, we always call seek() to the last seek > > position. > > > The seek position is there because user may switch tabs, or tap on the seek > > bar > > > when player is not prepared. By default, seek(0) is called. > > > For 4.2, seek will not cause any problems for live stream. > > > For 4.3, it will stuck the player in an error state. > > > > > > On 2013/08/09 17:03:19, xhwang wrote: > > > > Newbie question: in what situation could these conditions be true? > Shouldn't > > > the > > > > player/ui who issues the seek request check for these conditions? In that > > > case, > > > > we only need to DCHECK here. > > > > > > > Thanks for the explanation. So if time < base::TimeDelta(), can we > > DCHECK_EQ(time, -1)? > > Also thanks for asking and answering. Another newbie question: Why are we using > -1 literal rather than a well-defined constant for this in code? Like > kInfiniteDuration and kNoTimeStaamp, something like kSeekingInHLS??
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/22605013/19001
Message was sent while issue was closed.
Change committed as 216786 |
