Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22914021:
Logged information from BufferedDataSource including (Closed)
|
Created:
7 years, 4 months ago by Ty Overby Modified:
7 years, 3 months ago Reviewers:
scherkus (not reviewing) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionLogged information from BufferedDataSource including
* Content Length
* If the media is being streamed
* Same Origin Status
* CORS Status
* HTTP Range support
BUG=260005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222184
Patch Set 1 #Patch Set 2 : #Patch Set 3 : added comments #
Total comments: 11
Patch Set 4 : moved to success branch #Patch Set 5 : moved logging calls to success branch #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:170: typedef base::Callback<void(bool)> InitializeCB; scherkus@: How do I get rid of this line? I threw it in there because without it, the code that I wrote won't compile, but existing code (see line 140 in this file) worked just fine from the typedef included from buffered_data_source.h
https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:400: media_log_->SetIntegerProperty("total_bytes", total_bytes_); note that total_bytes_ might be -1 (kPositionNotSpecified) did you intend to log that? Also we run into the silliness with sizes larger than 2G here, so may have to use strings This information is also duplicated by FFmpegDemuxer (filesize_in_bytes) -- we should decide which one we want to keep (FYI FFD gets that information from BDS via BlockingUrlProtocol) https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:415: UpdateHostState_Locked(); similar to GpuVideoDecoder, I'd do away w/ wrapping the callback and tuck all the logging inside this success branch this section of code is the only place where we run init_cb_, so if it's successful we can go ahead with logging https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:462: media_log_->SetIntegerProperty("read_retries", read_op_->retries()); I wouldn't bother with this one -- ReadCallback() is every single time we read (1000s of time for the lifetime of a playback), so this value would be constantly overwritten with new values
https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:202: << "Initialize() must complete before calling HasSingleOrigin()"; if you're worried about the DCHECKs() for logging purposes you can get around it one of two ways: 1) call loader_->HasSingleOrigin() yourself 2) copy init_cb_ locally inside StartCallback() and clear init_cb_, then call HasSingleOrigin()
https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:202: << "Initialize() must complete before calling HasSingleOrigin()"; On 2013/08/21 20:31:12, scherkus wrote: > if you're worried about the DCHECKs() for logging purposes you can get around it > one of two ways: > 1) call loader_->HasSingleOrigin() yourself > 2) copy init_cb_ locally inside StartCallback() and clear init_cb_, then call > HasSingleOrigin() Option 1 worked Done. https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:400: media_log_->SetIntegerProperty("total_bytes", total_bytes_); On 2013/08/21 20:28:41, scherkus wrote: > note that total_bytes_ might be -1 (kPositionNotSpecified) > > did you intend to log that? > > Also we run into the silliness with sizes larger than 2G here, so may have to > use strings > > This information is also duplicated by FFmpegDemuxer (filesize_in_bytes) -- we > should decide which one we want to keep (FYI FFD gets that information from BDS > via BlockingUrlProtocol) That's right; I forgot that we already had it from FFD. I'll remove it here. https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:400: media_log_->SetIntegerProperty("total_bytes", total_bytes_); On 2013/08/21 20:28:41, scherkus wrote: > note that total_bytes_ might be -1 (kPositionNotSpecified) > > did you intend to log that? > > Also we run into the silliness with sizes larger than 2G here, so may have to > use strings > > This information is also duplicated by FFmpegDemuxer (filesize_in_bytes) -- we > should decide which one we want to keep (FYI FFD gets that information from BDS > via BlockingUrlProtocol) Done. https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:415: UpdateHostState_Locked(); On 2013/08/21 20:28:41, scherkus wrote: > similar to GpuVideoDecoder, I'd do away w/ wrapping the callback and tuck all > the logging inside this success branch > > this section of code is the only place where we run init_cb_, so if it's > successful we can go ahead with logging Ok, I'll see if I can avoid the DCHECK and put it here. https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:415: UpdateHostState_Locked(); On 2013/08/21 20:28:41, scherkus wrote: > similar to GpuVideoDecoder, I'd do away w/ wrapping the callback and tuck all > the logging inside this success branch > > this section of code is the only place where we run init_cb_, so if it's > successful we can go ahead with logging Done. https://codereview.chromium.org/22914021/diff/13001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:462: media_log_->SetIntegerProperty("read_retries", read_op_->retries()); It's only called if there is a miss I believe, and you did say in the design doc that you wanted the number of retries. I'll leave this one up to you.
few nits -- don't forget to update the CL description based on what we finally end up logging in this class https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:373: (total_bytes_ == kPositionNotSpecified || !loader_->range_supported()); I'd actually have total bytes logged in this class and *not* in FFmpegDemuxer (why? because BDS knows much more about the total bytes value) can you add a log line to the success block below and remove the one from FFmpegDemuxer? https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:387: media_log_->SetBooleanProperty("pass_cors_access_check", nit: s/pass/passed/ https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:389: // HTTP range header nit: remove this comment https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:439: media_log_->SetIntegerProperty("read_retries", read_op_->retries()); for now you can remove it ... it's a lower priority one vs. other ones (main reason being, the other logs are captured and logged on the initial connection, but this one can be set anytime we seek and force a new HTTP connection ... adding per-connection logging should be tackled as a separate problem)
Wow, I totally forgot about this CL. https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:373: (total_bytes_ == kPositionNotSpecified || !loader_->range_supported()); On 2013/08/22 01:32:58, scherkus wrote: > I'd actually have total bytes logged in this class and *not* in FFmpegDemuxer > (why? because BDS knows much more about the total bytes value) > > can you add a log line to the success block below and remove the one from > FFmpegDemuxer? Do you still want this done? https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:387: media_log_->SetBooleanProperty("pass_cors_access_check", On 2013/08/22 01:32:59, scherkus wrote: > nit: s/pass/passed/ Done. https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:389: // HTTP range header On 2013/08/22 01:32:59, scherkus wrote: > nit: remove this comment Done. https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:439: media_log_->SetIntegerProperty("read_retries", read_op_->retries()); On 2013/08/22 01:32:59, scherkus wrote: > for now you can remove it ... it's a lower priority one vs. other ones > > (main reason being, the other logs are captured and logged on the initial > connection, but this one can be set anytime we seek and force a new HTTP > connection ... adding per-connection logging should be tackled as a separate > problem) Done.
https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:373: (total_bytes_ == kPositionNotSpecified || !loader_->range_supported()); On 2013/09/09 22:40:13, Ty Overby wrote: > On 2013/08/22 01:32:58, scherkus wrote: > > I'd actually have total bytes logged in this class and *not* in FFmpegDemuxer > > (why? because BDS knows much more about the total bytes value) > > > > can you add a log line to the success block below and remove the one from > > FFmpegDemuxer? > > Do you still want this done? Ah so I'm *not* crazy! I swore I saw you add MediaLog code to BDS but couldn't find it :) Yeah, if you plan on landing this can you remove the total_bytes from FFmpegDemuxer?
also update your CL description (e.g., "Connection Retry Count" isn't logged)
https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... File content/renderer/media/buffered_data_source.cc (right): https://codereview.chromium.org/22914021/diff/23001/content/renderer/media/bu... content/renderer/media/buffered_data_source.cc:373: (total_bytes_ == kPositionNotSpecified || !loader_->range_supported()); On 2013/09/09 22:42:58, scherkus wrote: > On 2013/09/09 22:40:13, Ty Overby wrote: > > On 2013/08/22 01:32:58, scherkus wrote: > > > I'd actually have total bytes logged in this class and *not* in > FFmpegDemuxer > > > (why? because BDS knows much more about the total bytes value) > > > > > > can you add a log line to the success block below and remove the one from > > > FFmpegDemuxer? > > > > Do you still want this done? > > Ah so I'm *not* crazy! I swore I saw you add MediaLog code to BDS but couldn't > find it :) > > Yeah, if you plan on landing this can you remove the total_bytes from > FFmpegDemuxer? Haha, yeah, I do plan on landing this. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/22914021/33001
Message was sent while issue was closed.
Change committed as 222184 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
