Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796193002:
fix canplaythrough (Closed)
|
Created:
3 years, 8 months ago by hubbe Modified:
3 years, 7 months ago CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionfix canplaythrough
This makes canplaythrough follow the spec.
It fires when we estimate that we have enough data to play to the
end, or when we stop downloading because the buffer is full.
We do this by keeping a time series in BufferedDataSourceHostImpl
which is used to estimate the download speed over the last 10 seconds.
BUG=73609
Review-Url: https://codereview.chromium.org/2796193002
Cr-Commit-Position: refs/heads/master@{#469797}
Committed: https://chromium.googlesource.com/chromium/src/+/b2d3efd2ec18ffd52c3dacfece9862366b565a3d
Patch Set 1 #Patch Set 2 : event-driven, better rate estimator #Patch Set 3 : test compile fix #
Total comments: 16
Patch Set 4 : comments addressed #Patch Set 5 : test added #Patch Set 6 : comments addressed #
Total comments: 35
Patch Set 7 : comments addressed #
Total comments: 1
Patch Set 8 : merged #Patch Set 9 : shortcut for fully buffered data #Patch Set 10 : no bytes left to download -> canplay #Patch Set 11 : not downloading -> canplaythrough = true #
Total comments: 18
Patch Set 12 : comments addressed #Patch Set 13 : tick clock fixes #Patch Set 14 : merged #Patch Set 15 : merged #
Total comments: 8
Patch Set 16 : nits fixed, now behind a flag #Patch Set 17 : merged #
Total comments: 2
Patch Set 18 : kSpecCompliantCanPlayThrough #Messages
Total messages: 114 (77 generated)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
hubbe@chromium.org changed reviewers: + dalecurtis@chromium.org, sandersd@chromium.org
I suspect I will need to update some tests....
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Can you update the CL description with some more details on what you're doing here? I.e. why you've chosen a decay rate, and the value etc. Since clients use this to trigger playback, this should be event driven and not based on the didLoadingProgress() timer -- which runs every 350ms per spec.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== fix canplaythrough This makes canplaythrough follow the spec. It fires when we estimate that we have enough data to play to the end, or when we stop downloading because the buffer is full. BUG=73609 ========== to ========== fix canplaythrough This makes canplaythrough follow the spec. It fires when we estimate that we have enough data to play to the end, or when we stop downloading because the buffer is full. We do this by keeping a time series in BufferedDataSourceHostImpl which is used to estimate the download speed over the last 10 seconds. BUG=73609 ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
On 2017/04/05 00:18:07, DaleCurtis wrote: > Can you update the CL description with some more details on what you're doing > here? I.e. why you've chosen a decay rate, and the value etc. Since clients use > this to trigger playback, this should be event driven and not based on the > didLoadingProgress() timer -- which runs every 350ms per spec. PTAL. It's now event driven. I did update the CL description a little, and I also added more comments to the code.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
dalecurtis@chromium.org changed reviewers: + chcunningham@chromium.org
Need to look more closely, but I think this will trample the have_enough vs have_nothing states coming from the decoders. +chcunningham for review.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
Left a couple of comments while reading the CL. Also, maybe you could have a better CL title? :) https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:119: double download_rate = 1.0E20; Why 1e20 and not a negative value instead? https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1671: SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData); If I understand correctly, that means that we will advertise as being able to play through if we have future data and not downloading? Why?
https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:60: download_history_.back() = std::make_pair(now, bytes_so_far); why not always push_back? iiuc this is overwriting the last entry, so we loose some recent data that we'd like to include in the computation of download rate. you pop of the front a few lines below, so it should be fine for the list to temporarily grow beyond its max entries. https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:120: for (int i = 0; i < std::min<int>(20, download_history_.size() - 3); i++) { I don't follow the min(20, ...). If you only want the first 20 entries why keep 1024? Also, why bias so heavily toward the first 20? Also, why size() -3 ? https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl_unittest.cc:13: BufferedDataSourceHostImplTest() : host_(base::Closure()) {} Add a test? https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1287: SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData); Do we need this logic both here and in didLoadingProgress? Can we just have this on? I'm not a fan of how didLoadingProgress has sideffects. https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1292: return buffered_data_source_host_.CanPlayThrough( What about MSE (no buffered_data_source_host_)?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I re-wrote some parts of how the download_history_ queue is handled and added more comments. https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:60: download_history_.back() = std::make_pair(now, bytes_so_far); On 2017/04/06 16:55:41, chcunningham wrote: > why not always push_back? iiuc this is overwriting the last entry, so we loose > some recent data that we'd like to include in the computation of download rate. > you pop of the front a few lines below, so it should be fine for the list to > temporarily grow beyond its max entries. We don't loose any data, we just loose precision in time. This data is like a staircase, and doing the overwriting just makes the steps bigger. (However, I now realize that this code will make the step twice as big, but it cannot make the steps 3 times as big, or 4 times as big, should the need arise.... https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:119: double download_rate = 1.0E20; On 2017/04/06 11:32:56, mlamouri wrote: > Why 1e20 and not a negative value instead? Because I use std::min() below. https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:120: for (int i = 0; i < std::min<int>(20, download_history_.size() - 3); i++) { On 2017/04/06 16:55:41, chcunningham wrote: > I don't follow the min(20, ...). If you only want the first 20 entries why keep > 1024? Also, why bias so heavily toward the first 20? > > Also, why size() -3 ? Each entry contains the cumulative downloaded bytes, so last_entry - first_entry is enough to know how much we've downloaded in the last ~10 seconds. The 20 is simply to deal with bursty data. size() - 3 is to avoid estimating based on too few data points. https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl_unittest.cc:13: BufferedDataSourceHostImplTest() : host_(base::Closure()) {} On 2017/04/06 16:55:41, chcunningham wrote: > Add a test? Done. https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1287: SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData); On 2017/04/06 16:55:41, chcunningham wrote: > Do we need this logic both here and in didLoadingProgress? Can we just have this > on? I'm not a fan of how didLoadingProgress has sideffects. I can remove it from didLoadingProgress() https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1292: return buffered_data_source_host_.CanPlayThrough( On 2017/04/06 16:55:41, chcunningham wrote: > What about MSE (no buffered_data_source_host_)? There is always a buffered_data_source_host_, but of course it's not used. Added code to make it behave as before for MSE. https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1671: SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData); On 2017/04/06 11:32:56, mlamouri wrote: > If I understand correctly, that means that we will advertise as being able to > play through if we have future data and not downloading? Why? That's what the standard says to do when the buffer is full.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I think we should land a Media.TimeUntilPlayback UMA before this, which records the time from first load complete until the first playback request. This way we'll get an idea for how long this increases the time until playback for sites. If this functions correctly we should also see reduction in the total count for Media.UnderflowDuration. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:11: const double kDownloadHistoryWindowSeconds = 10.0; Add comments detailing how these were chosen. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:36: if (!intersection.Empty()) { Drop single line {} https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:47: int64_t new_bytes = UnloadedBytesInInterval(Interval<int64_t>(start, end)); Seems like there might be a simpler calculation since this is always contiguous (i.e. we don't remove ranges), but this is exhaustively correct. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:70: // Drop entries that are too old. DCHECK_GE(size, 1u) above. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:76: if (!progress_cb_.is_null()) { Drop single line {} Require so you don't have to conditionally check? There's only one caller of this api... https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:119: if (download_history_.size() < 5) Comment. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:130: double downloaded_seconds = base::TimeDelta? https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:136: std::min(download_rate, downloaded_bytes / downloaded_seconds); Then just InSecondsF() here. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:138: if (download_rate == 1.0E20) Ternary. Extract value to constant with explanation since it's used twice. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:147: if (!total_bytes_ || media_duration <= base::TimeDelta()) DCHECK_GE(playback_rate, 0); Also keep in mind media_duration might be kInfiniteDuration() https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:157: if (download_rate == 0.0) Seems a risky test due to precision. <= 0? https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:81: // Pruned to contain roughtly the last 10 seconds of data. roughly. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl_unittest.cc:6: #include "base/bind.h" add line above. https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:851: if (highest_ready_state_ < ReadyState::ReadyStateHaveFutureData) { This whole block can actually be moved to OnProgress() now without the first conditional. https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1288: if (chunk_demuxer_) Ternary? https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1315: if (CanPlayThrough()) { drop one line {} https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1668: if (ready_state_ == ReadyState::ReadyStateHaveFutureData && !is_downloading) Don't think this function should call SetReadyState; since it's otherwise only SetNetworkState. Can we just let this happen via the progress callback above? https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:458: // Called when loading progresses. Needs comment differentiating it from didLoadingProgress.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/buffered_da... media/blink/buffered_data_source_host_impl.cc:119: double download_rate = 1.0E20; On 2017/04/06 at 21:25:38, hubbe wrote: > On 2017/04/06 11:32:56, mlamouri wrote: > > Why 1e20 and not a negative value instead? > > Because I use std::min() below. Missed that. Can you use INT_MAX? `std::numeric_limits<double>::max()` to be precise :) https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1671: SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData); On 2017/04/06 at 21:25:38, hubbe wrote: > On 2017/04/06 11:32:56, mlamouri wrote: > > If I understand correctly, that means that we will advertise as being able to > > play through if we have future data and not downloading? Why? > > That's what the standard says to do when the buffer is full. FWIW, what I understood by reading the code is that we are no longer downloading not that the buffer was full. The buffer being full makes sense but do we stop downloading only if the buffer is full? Maybe add a comment if this can only happen when the buffer is full?
still need to fix tests... https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:11: const double kDownloadHistoryWindowSeconds = 10.0; On 2017/04/06 22:51:39, DaleCurtis wrote: > Add comments detailing how these were chosen. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:36: if (!intersection.Empty()) { On 2017/04/06 22:51:39, DaleCurtis wrote: > Drop single line {} Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:47: int64_t new_bytes = UnloadedBytesInInterval(Interval<int64_t>(start, end)); On 2017/04/06 22:51:39, DaleCurtis wrote: > Seems like there might be a simpler calculation since this is always contiguous > (i.e. we don't remove ranges), but this is exhaustively correct. Seeks cause non-continuous ranges. The removed code was simpler, but since I need the total bytes anyways... https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:70: // Drop entries that are too old. On 2017/04/06 22:51:39, DaleCurtis wrote: > DCHECK_GE(size, 1u) above. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:76: if (!progress_cb_.is_null()) { On 2017/04/06 22:51:39, DaleCurtis wrote: > Drop single line {} Require so you don't have to conditionally check? There's > only one caller of this api... Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:119: if (download_history_.size() < 5) On 2017/04/06 22:51:39, DaleCurtis wrote: > Comment. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:130: double downloaded_seconds = On 2017/04/06 22:51:39, DaleCurtis wrote: > base::TimeDelta? Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:136: std::min(download_rate, downloaded_bytes / downloaded_seconds); On 2017/04/06 22:51:39, DaleCurtis wrote: > Then just InSecondsF() here. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:138: if (download_rate == 1.0E20) On 2017/04/06 22:51:39, DaleCurtis wrote: > Ternary. Extract value to constant with explanation since it's used twice. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:157: if (download_rate == 0.0) On 2017/04/06 22:51:39, DaleCurtis wrote: > Seems a risky test due to precision. <= 0? Sure. This was just to avoid a division by zero when no estimate is returned from DownloadRate() though. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:81: // Pruned to contain roughtly the last 10 seconds of data. On 2017/04/06 22:51:39, DaleCurtis wrote: > roughly. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl_unittest.cc:6: #include "base/bind.h" On 2017/04/06 22:51:40, DaleCurtis wrote: > add line above. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:851: if (highest_ready_state_ < ReadyState::ReadyStateHaveFutureData) { On 2017/04/06 22:51:40, DaleCurtis wrote: > This whole block can actually be moved to OnProgress() now without the first > conditional. Done. https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1288: if (chunk_demuxer_) On 2017/04/06 22:51:40, DaleCurtis wrote: > Ternary? Makes the expression too long I think. It's already long... https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1315: if (CanPlayThrough()) { On 2017/04/06 22:51:40, DaleCurtis wrote: > drop one line {} How about ternary? :) https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1668: if (ready_state_ == ReadyState::ReadyStateHaveFutureData && !is_downloading) On 2017/04/06 22:51:40, DaleCurtis wrote: > Don't think this function should call SetReadyState; since it's otherwise only > SetNetworkState. Can we just let this happen via the progress callback above? No, we won't get progress callbacks after we stop downloading... https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2796193002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:458: // Called when loading progresses. On 2017/04/06 22:51:40, DaleCurtis wrote: > Needs comment differentiating it from didLoadingProgress. Done.
I think this loogs good, but I'm about to go OOO for a week so I defer to Dale and others. Dale mentioned worry about trampling have_current underflow logic - AFAICT this isn't a problem. Should also be checked by tests I added - keep an eye out for failing WebKit/LayoutTests/http/tests/media/video-play-stall.html I'd double check that you haven't regressed it... but I forget which bot runs the layout tests. https://codereview.chromium.org/2796193002/diff/120001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/120001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:139: for (int i = 0; i < std::min<int>(20, download_history_.size() - 3); i++) { I follow the 20 and the 3 now, but it might help to add still more comments to explain these ;)
On 2017/04/08 at 00:52:46, chcunningham wrote: > I think this loogs good, but I'm about to go OOO for a week so I defer to Dale and others. > > Dale mentioned worry about trampling have_current underflow logic - AFAICT this isn't a problem. Should also be checked by tests I added - keep an eye out for failing > > WebKit/LayoutTests/http/tests/media/video-play-stall.html > > I'd double check that you haven't regressed it... but I forget which bot runs the layout tests. All bots running tests should run the layout tests (webkit_tests). There are a ton of tests failing so I didn't check if this one in particular fails. You can look at linux_chromium_rel_ng for example.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ALL YOUR TEST ARE BELONG TO US PTAL
Did you notice any changes in time to play for telemetry tests? I don't remember if we have any http tests there now or if they all use file:// https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:14: const double kDownloadHistoryWindowSeconds = 10.0; static or constexpr https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:26: const base::Closure& progress_cb) I think we're supposed to use std::move with callbacks now. https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:83: base::TimeDelta::FromSecondsD(kDownloadHistoryWindowSeconds)) { Construct outside of the loop? https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:130: if (download_history_.size() < 5) The code above seems to indicate it's possible to end up with a contiguous range (always replace back) which would always fail this check? https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl_unittest.cc:82: Needs more tests for AddBufferedByteRange, DownloadRate() and CanPlayThrough(). https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1297: // TODO(sandersd): Should this be on the same stack? It might be Delete todo now. https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1301: } else if?
https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:82: std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; I'm a bit worried about the size and complexity of this deque. I think it should be abstracted into its own class, and I would propose implementing it using a fixed number of tagged bins instead. Specifically, something like this: struct { int64_t time; int64_t bytes; } bins[10]; (This example structure support second-granularity tracking over ten seconds in a fixed amount of memory; each transfer just gets counted into bin[now % 10].)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(most) comments addressed. Going to do a telemetry run now. PTAL https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:14: const double kDownloadHistoryWindowSeconds = 10.0; On 2017/04/25 21:45:27, DaleCurtis wrote: > static or constexpr Done. https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:26: const base::Closure& progress_cb) On 2017/04/25 21:45:27, DaleCurtis wrote: > I think we're supposed to use std::move with callbacks now. Like this? https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:83: base::TimeDelta::FromSecondsD(kDownloadHistoryWindowSeconds)) { On 2017/04/25 21:45:27, DaleCurtis wrote: > Construct outside of the loop? Done. https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:130: if (download_history_.size() < 5) On 2017/04/25 21:45:28, DaleCurtis wrote: > The code above seems to indicate it's possible to end up with a contiguous range > (always replace back) which would always fail this check? It shouldn't be possible. Over-writing the last entry only happens if the difference between the last two entries is small, and that difference grows every time we over-write the last entry. https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:82: std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; On 2017/04/25 22:00:57, sandersd wrote: > I'm a bit worried about the size and complexity of this deque. I think it should > be abstracted into its own class, and I would propose implementing it using a > fixed number of tagged bins instead. > > Specifically, something like this: struct { int64_t time; int64_t bytes; } > bins[10]; > > (This example structure support second-granularity tracking over ten seconds in > a fixed amount of memory; each transfer just gets counted into bin[now % 10].) Given that each entry represents at least 1000 bytes, the overhead seems pretty small. I didn't have a lot of luck with reducing the complexity, and I don't think doing the modular math myself is simpler than letting deque do it for me. As for breaking it out: I could, but personally I don't think it's worth it. Dale, what do you think? https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl_unittest.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl_unittest.cc:82: On 2017/04/25 21:45:28, DaleCurtis wrote: > Needs more tests for AddBufferedByteRange, DownloadRate() and CanPlayThrough(). More tests added. https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1297: // TODO(sandersd): Should this be on the same stack? It might be On 2017/04/25 21:45:28, DaleCurtis wrote: > Delete todo now. Done. https://codereview.chromium.org/2796193002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1301: } On 2017/04/25 21:45:28, DaleCurtis wrote: > else if? Done.
https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:82: std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; On 2017/04/25 at 23:47:03, hubbe wrote: > On 2017/04/25 22:00:57, sandersd wrote: > > I'm a bit worried about the size and complexity of this deque. I think it should > > be abstracted into its own class, and I would propose implementing it using a > > fixed number of tagged bins instead. > > > > Specifically, something like this: struct { int64_t time; int64_t bytes; } > > bins[10]; > > > > (This example structure support second-granularity tracking over ten seconds in > > a fixed amount of memory; each transfer just gets counted into bin[now % 10].) > > Given that each entry represents at least 1000 bytes, the overhead seems pretty small. I didn't have a lot of luck with reducing the complexity, and I don't think doing the modular math myself is simpler than letting deque do it for me. > > As for breaking it out: I could, but personally I don't think it's worth it. Dale, what do you think? What does the size of this look like currently? I don't think this class does much so I'm happy to have it in here so long as it's well tested. I share Dan's concerns about the growth of it though, so having an idea of what it looks like after 15 minutes of playback would be interesting.
https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:82: std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; On 2017/04/26 00:11:26, DaleCurtis wrote: > On 2017/04/25 at 23:47:03, hubbe wrote: > > On 2017/04/25 22:00:57, sandersd wrote: > > > I'm a bit worried about the size and complexity of this deque. I think it > should > > > be abstracted into its own class, and I would propose implementing it using > a > > > fixed number of tagged bins instead. > > > > > > Specifically, something like this: struct { int64_t time; int64_t bytes; } > > > bins[10]; > > > > > > (This example structure support second-granularity tracking over ten seconds > in > > > a fixed amount of memory; each transfer just gets counted into bin[now % > 10].) > > > > Given that each entry represents at least 1000 bytes, the overhead seems > pretty small. I didn't have a lot of luck with reducing the complexity, and I > don't think doing the modular math myself is simpler than letting deque do it > for me. > > > > As for breaking it out: I could, but personally I don't think it's worth it. > Dale, what do you think? > > What does the size of this look like currently? I don't think this class does > much so I'm happy to have it in here so long as it's well tested. I share Dan's > concerns about the growth of it though, so having an idea of what it looks like > after 15 minutes of playback would be interesting. There is a maximum of 1000 entries right now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/04/26 00:24:27, hubbe wrote: > https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... > File media/blink/buffered_data_source_host_impl.h (right): > > https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... > media/blink/buffered_data_source_host_impl.h:82: > std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; > On 2017/04/26 00:11:26, DaleCurtis wrote: > > On 2017/04/25 at 23:47:03, hubbe wrote: > > > On 2017/04/25 22:00:57, sandersd wrote: > > > > I'm a bit worried about the size and complexity of this deque. I think it > > should > > > > be abstracted into its own class, and I would propose implementing it > using > > a > > > > fixed number of tagged bins instead. > > > > > > > > Specifically, something like this: struct { int64_t time; int64_t bytes; } > > > > bins[10]; > > > > > > > > (This example structure support second-granularity tracking over ten > seconds > > in > > > > a fixed amount of memory; each transfer just gets counted into bin[now % > > 10].) > > > > > > Given that each entry represents at least 1000 bytes, the overhead seems > > pretty small. I didn't have a lot of luck with reducing the complexity, and I > > don't think doing the modular math myself is simpler than letting deque do it > > for me. > > > > > > As for breaking it out: I could, but personally I don't think it's worth it. > > Dale, what do you think? > > > > What does the size of this look like currently? I don't think this class does > > much so I'm happy to have it in here so long as it's well tested. I share > Dan's > > concerns about the growth of it though, so having an idea of what it looks > like > > after 15 minutes of playback would be interesting. > > There is a maximum of 1000 entries right now. This implies 16KiB of heap allocation, so on the plus side we don't have to worry about the excessive size of std::deque on some platforms.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/26 00:28:31, sandersd wrote: > On 2017/04/26 00:24:27, hubbe wrote: > > > https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... > > File media/blink/buffered_data_source_host_impl.h (right): > > > > > https://codereview.chromium.org/2796193002/diff/200001/media/blink/buffered_d... > > media/blink/buffered_data_source_host_impl.h:82: > > std::deque<std::pair<base::TimeTicks, uint64_t>> download_history_; > > On 2017/04/26 00:11:26, DaleCurtis wrote: > > > On 2017/04/25 at 23:47:03, hubbe wrote: > > > > On 2017/04/25 22:00:57, sandersd wrote: > > > > > I'm a bit worried about the size and complexity of this deque. I think > it > > > should > > > > > be abstracted into its own class, and I would propose implementing it > > using > > > a > > > > > fixed number of tagged bins instead. > > > > > > > > > > Specifically, something like this: struct { int64_t time; int64_t bytes; > } > > > > > bins[10]; > > > > > > > > > > (This example structure support second-granularity tracking over ten > > seconds > > > in > > > > > a fixed amount of memory; each transfer just gets counted into bin[now % > > > 10].) > > > > > > > > Given that each entry represents at least 1000 bytes, the overhead seems > > > pretty small. I didn't have a lot of luck with reducing the complexity, and > I > > > don't think doing the modular math myself is simpler than letting deque do > it > > > for me. > > > > > > > > As for breaking it out: I could, but personally I don't think it's worth > it. > > > Dale, what do you think? > > > > > > What does the size of this look like currently? I don't think this class > does > > > much so I'm happy to have it in here so long as it's well tested. I share > > Dan's > > > concerns about the growth of it though, so having an idea of what it looks > > like > > > after 15 minutes of playback would be interesting. > > > > There is a maximum of 1000 entries right now. > > This implies 16KiB of heap allocation, so on the plus side we don't have to > worry about the excessive size of std::deque on some platforms. So we're back to all tests passing again. I'm not sure if there are flaky tests or if there was some other code changes that made things fail for a while. I could try to re-write the bandwidth estimator, but I don't think the current implementation is really that bad. Also, i don't think the proposed 10 1-second buckets is actually accurate enough. We want canplaythrough to fire as soon as possible, which is often in less than a second.
Per https://codereview.chromium.org/2796193002#msg34 we should land a UMA to help us understand how long this delays playback by on average. Also, if this is working correctly, we should see a reduction in the count of Media.UnderflowDuration events. Ideas for the metric: - Add HTMLME metric which tracks load -> play for autoplayable elements. - Add metric which tracks load -> canplaythrough. If we launch this as a dev/canary experiment we can see how this impacts the population across mobile, dev, etc with those metrics. https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:73: // If the difference between the last entry and the second to last entry is Do your tests hit both of these paths? https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:80: download_history_.push_back(std::make_pair(now, bytes_so_far)); emplace_back? https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:160: if (!total_bytes_ || media_duration <= base::TimeDelta() || needs {} for multiline if. https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:42: BufferedDataSourceHostImpl(base::Closure&& progress_cb, no && necessary. base::RepeatingClosure?
(otherwise CL lg2m, but this has the potential for a lot of fallout, so we need to have a measurement plan here).
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/01 19:06:16, DaleCurtis wrote: > (otherwise CL lg2m, but this has the potential for a lot of fallout, so we need > to have a measurement plan here). I wonder if the counts we have already are sufficient? Adding histograms that tells us how much autoplay is delayed just doesn't seem very actionable to me, as we already know that autoplay will be delayed to some degree. What we're worried about is cases where autoplay stops working, or gets delayed a LOT. There are already counters for autoplay, and they should tell us if something is going catastrophically wrong I think. Measuring time to canplaythrough would essentially be a new metric, so that wouldn't be very actionable either. Would it make sense to launch the experiment as-is (without new metrics) and then try to figure out what additional data we need? Maybe what we need is a counter for when autoplay is enabled, but doesn't actually trigger?
On 2017/05/01 at 21:19:48, hubbe wrote: > On 2017/05/01 19:06:16, DaleCurtis wrote: > > (otherwise CL lg2m, but this has the potential for a lot of fallout, so we need > > to have a measurement plan here). > > I wonder if the counts we have already are sufficient? What counts? > > Adding histograms that tells us how much autoplay is delayed just doesn't seem very actionable to me, > as we already know that autoplay will be delayed to some degree. What we're worried about is cases where > autoplay stops working, or gets delayed a LOT. There are already counters for autoplay, and they should tell > us if something is going catastrophically wrong I think. Ditto. I don't know of any ms based timing. > > Measuring time to canplaythrough would essentially be a new metric, so that wouldn't be very actionable either. > > Would it make sense to launch the experiment as-is (without new metrics) and then try to figure out what additional > data we need? I think I'd agree if there were actually metrics, but I don't know which ones you're talking about and can't find any. > > Maybe what we need is a counter for when autoplay is enabled, but doesn't actually trigger?
On Mon, May 1, 2017 at 2:22 PM, <dalecurtis@chromium.org> wrote: > On 2017/05/01 at 21:19:48, hubbe wrote: > > On 2017/05/01 19:06:16, DaleCurtis wrote: > > > (otherwise CL lg2m, but this has the potential for a lot of fallout, > so we > need > > > to have a measurement plan here). > > > > I wonder if the counts we have already are sufficient? > > What counts? > > Media.{Video,Audio}.Autoplay (and the rest of the counts in AutoPlayUmaHelper.cpp) > > > > Adding histograms that tells us how much autoplay is delayed just > doesn't seem > very actionable to me, > > as we already know that autoplay will be delayed to some degree. What > we're > worried about is cases where > > autoplay stops working, or gets delayed a LOT. There are already > counters for > autoplay, and they should tell > > us if something is going catastrophically wrong I think. > > Ditto. I don't know of any ms based timing. > > > > > Measuring time to canplaythrough would essentially be a new metric, so > that > wouldn't be very actionable either. > > > > Would it make sense to launch the experiment as-is (without new metrics) > and > then try to figure out what additional > > data we need? > > I think I'd agree if there were actually metrics, but I don't know which > ones > you're talking about and can't find any. > > > > > Maybe what we need is a counter for when autoplay is enabled, but doesn't > actually trigger? > > > > https://codereview.chromium.org/2796193002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.cc (right): https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:73: // If the difference between the last entry and the second to last entry is On 2017/05/01 19:05:52, DaleCurtis wrote: > Do your tests hit both of these paths? Does now. https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:80: download_history_.push_back(std::make_pair(now, bytes_so_far)); On 2017/05/01 19:05:52, DaleCurtis wrote: > emplace_back? Done. https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.cc:160: if (!total_bytes_ || media_duration <= base::TimeDelta() || On 2017/05/01 19:05:52, DaleCurtis wrote: > needs {} for multiline if. Done. https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... File media/blink/buffered_data_source_host_impl.h (right): https://codereview.chromium.org/2796193002/diff/280001/media/blink/buffered_d... media/blink/buffered_data_source_host_impl.h:42: BufferedDataSourceHostImpl(base::Closure&& progress_cb, On 2017/05/01 19:05:52, DaleCurtis wrote: > no && necessary. Yuck, implicit move semantics is ugly. (but Done) > base::RepeatingClosure? Ok.
On 2017/05/01 at 21:24:51, chromium-reviews wrote: > On Mon, May 1, 2017 at 2:22 PM, <dalecurtis@chromium.org> wrote: > > > On 2017/05/01 at 21:19:48, hubbe wrote: > > > On 2017/05/01 19:06:16, DaleCurtis wrote: > > > > (otherwise CL lg2m, but this has the potential for a lot of fallout, > > so we > > need > > > > to have a measurement plan here). > > > > > > I wonder if the counts we have already are sufficient? > > > > What counts? > > > > > Media.{Video,Audio}.Autoplay (and the rest of the counts in > AutoPlayUmaHelper.cpp) > Those are not timing based though so wouldn't show anything for this change?
On 2017/05/01 21:29:30, DaleCurtis wrote:
> On 2017/05/01 at 21:24:51, chromium-reviews wrote:
> > On Mon, May 1, 2017 at 2:22 PM, <mailto:dalecurtis@chromium.org> wrote:
> >
> > > On 2017/05/01 at 21:19:48, hubbe wrote:
> > > > On 2017/05/01 19:06:16, DaleCurtis wrote:
> > > > > (otherwise CL lg2m, but this has the potential for a lot of fallout,
> > > so we
> > > need
> > > > > to have a measurement plan here).
> > > >
> > > > I wonder if the counts we have already are sufficient?
> > >
> > > What counts?
> > >
> > >
> > Media.{Video,Audio}.Autoplay (and the rest of the counts in
> > AutoPlayUmaHelper.cpp)
> >
>
> Those are not timing based though so wouldn't show anything for this change?
They would if autoplay fails because of this change.
Timing-based metrics will show a change, but we already know that, so how will
we decide if the results are good or bad?
On 2017/05/01 at 21:31:01, hubbe wrote: > > They would if autoplay fails because of this change. > Timing-based metrics will show a change, but we already know that, so how will we decide if the results are good or bad? That's a pretty rare case. I.e., it will only catch bad network cases which passed the metadata state or complete errors in this change. However I'm less worried about complete failure and more worried about this code delaying unnecessarily. Ideally we want this code to optimize around minimizing delay and number of underflow events. We know there will be some delay, but we no idea of our existing profile or how this will change that. Is it 5s? 1s? 100ms? If this change took us from 500ms to 5s that'd be pretty worrying.
On 2017/05/01 21:42:08, DaleCurtis wrote: > On 2017/05/01 at 21:31:01, hubbe wrote: > > > > They would if autoplay fails because of this change. > > Timing-based metrics will show a change, but we already know that, so how will > we decide if the results are good or bad? > > That's a pretty rare case. I.e., it will only catch bad network cases which > passed the metadata state or complete errors in this change. However I'm less > worried about complete failure and more worried about this code delaying > unnecessarily. Ideally we want this code to optimize around minimizing delay and > number of underflow events. > > We know there will be some delay, but we no idea of our existing profile or how > this will change that. Is it 5s? 1s? 100ms? If this change took us from 500ms to > 5s that'd be pretty worrying. Well, the max should be ~10 seconds, since that's when we normally decide that we have enough and defer loading. I'll see if I can add a timed histogram...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/01 21:48:37, hubbe wrote: > On 2017/05/01 21:42:08, DaleCurtis wrote: > > On 2017/05/01 at 21:31:01, hubbe wrote: > > > > > > They would if autoplay fails because of this change. > > > Timing-based metrics will show a change, but we already know that, so how > will > > we decide if the results are good or bad? > > > > That's a pretty rare case. I.e., it will only catch bad network cases which > > passed the metadata state or complete errors in this change. However I'm less > > worried about complete failure and more worried about this code delaying > > unnecessarily. Ideally we want this code to optimize around minimizing delay > and > > number of underflow events. > > > > We know there will be some delay, but we no idea of our existing profile or > how > > this will change that. Is it 5s? 1s? 100ms? If this change took us from 500ms > to > > 5s that'd be pretty worrying. > > Well, the max should be ~10 seconds, since that's when we normally decide that > we have enough and defer loading. > I'll see if I can add a timed histogram... Ok, so we're going to have a metric for time to autoplay. I've made it so that this CL doesn't do anything until a flag is enabled. I'm not sure if we need a time-to-canplaythrough metric, as this would be the same as the autoplay metric (but include videos that don't autoplay.) Seems like next step is to check in this CL, and the metric one and then create an experiment. Unless there are more things to do on this CL?
lgtm then, thanks! https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... media/base/media_switches.h:100: MEDIA_EXPORT extern const base::Feature kCanPlayThrough; kSpecCompliantCanPlayThrough?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... media/base/media_switches.h:100: MEDIA_EXPORT extern const base::Feature kCanPlayThrough; On 2017/05/04 22:56:51, DaleCurtis_OOO_May_5_To_May23 wrote: > kSpecCompliantCanPlayThrough? Done.
https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2796193002/diff/320001/media/base/media_switc... media/base/media_switches.h:100: MEDIA_EXPORT extern const base::Feature kCanPlayThrough; On 2017/05/04 22:56:51, DaleCurtis_OOO_May_5_To_May23 wrote: > kSpecCompliantCanPlayThrough? Done.
Still lgtm for me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2796193002/#ps340001 (title: "kSpecCompliantCanPlayThrough")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494026273265560,
"parent_rev": "79e98da1bf1d0e04f66483524c432a7efbaf8ac4", "commit_rev":
"b2d3efd2ec18ffd52c3dacfece9862366b565a3d"}
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494026273265560,
"parent_rev": "79e98da1bf1d0e04f66483524c432a7efbaf8ac4", "commit_rev":
"b2d3efd2ec18ffd52c3dacfece9862366b565a3d"}
Message was sent while issue was closed.
Description was changed from ========== fix canplaythrough This makes canplaythrough follow the spec. It fires when we estimate that we have enough data to play to the end, or when we stop downloading because the buffer is full. We do this by keeping a time series in BufferedDataSourceHostImpl which is used to estimate the download speed over the last 10 seconds. BUG=73609 ========== to ========== fix canplaythrough This makes canplaythrough follow the spec. It fires when we estimate that we have enough data to play to the end, or when we stop downloading because the buffer is full. We do this by keeping a time series in BufferedDataSourceHostImpl which is used to estimate the download speed over the last 10 seconds. BUG=73609 Review-Url: https://codereview.chromium.org/2796193002 Cr-Commit-Position: refs/heads/master@{#469797} Committed: https://chromium.googlesource.com/chromium/src/+/b2d3efd2ec18ffd52c3dacfece98... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/b2d3efd2ec18ffd52c3dacfece98... |
