New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Oct 2011
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
link

Issue 99775: Initial BufferedResourceLoader objects are always kReadThenDefer

Reported by scherkus@chromium.org, Oct 10 2011 Project Member

Issue description

BufferedDataSource's constructor initializes preload_ to media::METADATA, which means that the first BufferedResourceLoader object always uses the kReadThenDefer strategy

Later on when WebKit tells us that the preload is actually AUTO, then we forward the calls through WebMediaPlayerImpl -> PipelineImpl ->  FFmpegDemuxer -> BufferedDataSource who sets preload_ but doesn't change the strategy

That means that the current BufferedResourceLoader remains in kReadThenDefer until something comes along to either call BRL::UpdateDeferStrategy() or the connection is restarted.

This should have a pretty significant impact on video start up latency.
 

Comment 1 by scherkus@chromium.org, Oct 10 2011

Labels: ReleaseBlock-Stable

Comment 2 by scherkus@chromium.org, Oct 11 2011

this one is bad but luckily patch isn't too invasive

http://codereview.chromium.org/8224028/

Comment 3 by oritm@chromium.org, Oct 11 2011

Does this repro on M14?

Comment 4 by alberto@google.com, Oct 11 2011

ImportantForVideo does not affect M14.

Comment 5 by bugdroid1@chromium.org, Oct 12 2011

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=105121

------------------------------------------------------------------------
r105121 | scherkus@chromium.org | Wed Oct 12 11:00:46 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_data_source.cc?r1=105121&r2=105120&pathrev=105121
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_data_source_unittest.cc?r1=105121&r2=105120&pathrev=105121
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_data_source.h?r1=105121&r2=105120&pathrev=105121
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_resource_loader_unittest.cc?r1=105121&r2=105120&pathrev=105121
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_resource_loader.h?r1=105121&r2=105120&pathrev=105121
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/media/buffered_resource_loader.cc?r1=105121&r2=105120&pathrev=105121

Numerous fixes to audio/video buffered resource loading.

This patch fixes a few related issues:
  1) Default loading strategy is now threshold-then-defer
  2) Specify more reasonable default bitrate/playback rate values
  3) Use a minimum buffer window size to prevent underflows on low bitrate content
  4) Remember bitrate/playback rate values between resource loaders

The default loading strategy of read-then-defer had a negative impact on initial latency as we were constantly deferring/undeferring the connection during the time when we need data the fastest.

While this change does result in loading a pinch more data for preload=metadata scenarios, it vastly improves the common preload=auto scenario.

BUG= 99775 
TEST=test_shell_tests

Review URL: http://codereview.chromium.org/8224028
------------------------------------------------------------------------

Comment 6 by scherkus@chromium.org, Oct 14 2011

Labels: Merge-Requested
Status: Fixed
also good to go!

Comment 7 by kareng@google.com, Oct 17 2011

Labels: -Merge-Requested Merge-Approved
does this mean it's confirmed?

Comment 8 by scherkus@chromium.org, Oct 17 2011

Status: Verified
yup!

Comment 9 by bugdroid1@chromium.org, Oct 17 2011

Project Member
Labels: -merge-approved merge-merged-874
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=105963

------------------------------------------------------------------------
r105963 | scherkus@chromium.org | Mon Oct 17 16:38:04 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/base/seekable_buffer.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_resource_loader_unittest.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/simple_data_source.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_resource_loader.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/file_data_source.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/ffmpeg_demuxer_unittest.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/ffmpeg_demuxer.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_data_source.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_data_source_unittest.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_data_source.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/base/filters.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/ffmpeg_demuxer.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/file_data_source.h?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/buffered_resource_loader.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/webkit/glue/media/simple_data_source.cc?r1=105963&r2=105962&pathrev=105963
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/base/mock_filters.h?r1=105963&r2=105962&pathrev=105963

Merge 105121 - Numerous fixes to audio/video buffered resource loading.

This patch fixes a few related issues:
  1) Default loading strategy is now threshold-then-defer
  2) Specify more reasonable default bitrate/playback rate values
  3) Use a minimum buffer window size to prevent underflows on low bitrate content
  4) Remember bitrate/playback rate values between resource loaders

The default loading strategy of read-then-defer had a negative impact on initial latency as we were constantly deferring/undeferring the connection during the time when we need data the fastest.

While this change does result in loading a pinch more data for preload=metadata scenarios, it vastly improves the common preload=auto scenario.

BUG= 99775 
TEST=test_shell_tests

Review URL: http://codereview.chromium.org/8224028
Review URL: http://codereview.chromium.org/8294025
------------------------------------------------------------------------

Comment 10 by scherkus@chromium.org, Oct 19 2011

Labels: -ImportantForVideo Hotlist-NeededForVideo

Comment 11 by bugdroid1@chromium.org, Oct 13 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 12 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Area-WebKit -Mstone-15 -Feature-Media-Network Cr-Content Cr-Internals-Media-Network M-15

Comment 13 by bugdroid1@chromium.org, Mar 13 2013

Project Member
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 14 by bugdroid1@chromium.org, Apr 6 2013

Project Member
Labels: -Cr-Content Cr-Blink

Sign in to add a comment