Log stream parsing errors in ChunkDemuxer (issue 633243002 by servolk@chromium.org)

[ser...@chromium.org]

Reviewers: wolenetz, vrk, DaleCurtis,

Message:
PTAL

Description:
Log stream parsing errors in ChunkDemuxer

BUG=

Please review this at https://codereview.chromium.org/633243002/

SVN Base: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+8, -2 lines):
M media/filters/chunk_demuxer.cc


Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index
505d9c1f69c6a8275ad437ce23bd7374b72ffb97..64e79bf7cd3ca11ead2a1fdf2b9bfe3aab0270fb
100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -330,10 +330,16 @@ bool SourceState::Append(

// TODO(wolenetz/acolwell): Curry and pass a NewBuffersCB here bound with
// append window and timestamp offset pointer. See
http://crbug.com/351454.
- bool err = stream_parser_->Parse(data, length);
+ bool result = stream_parser_->Parse(data, length);
+ if (!result) {
+ DVLOG(1) << __FUNCTION__ << ": stream parsing failed."
+ << " Data size=" << length
+ << " append_window_start=" << append_window_start.InSecondsF()
+ << " append_window_end=" << append_window_end.InSecondsF();
+ }
timestamp_offset_during_append_ = NULL;
init_segment_received_cb_.Reset();
- return err;
+ return result;
}

void SourceState::Abort(TimeDelta append_window_start,

[dalec...@chromium.org]

https://codereview.chromium.org/633243002/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/633243002/diff/1/media/filters/chunk_demuxer.cc#newcode335
media/filters/chunk_demuxer.cc:335: DVLOG(1) << __FUNCTION__ << ":
stream parsing failed."
Is this something better sent to MEDIA_LOG() ?

https://codereview.chromium.org/633243002/

[ser...@chromium.org]

On 2014/10/07 22:08:16, DaleCurtis wrote:

https://codereview.chromium.org/633243002/diff/1/media/filters/chunk_demuxer.cc
> File media/filters/chunk_demuxer.cc (right):


https://codereview.chromium.org/633243002/diff/1/media/filters/chunk_demuxer.cc#newcode335
> media/filters/chunk_demuxer.cc:335: DVLOG(1) << __FUNCTION__ << ": stream
> parsing failed."
> Is this something better sent to MEDIA_LOG() ?

Possibly. I looked at the existing logging in chunk demuxer and it's not
obvious
to me when one or the other should be preferred. I can see some things,
including errors, being reported to the media_log and some are being
reported
using DVLOG(1), for example the failure in ChunkDemuxerStream::Append and
SourceState::OnNewConfigs and ChunkDemuxerStream::Seek. Is there some kind
of
guideline explaining which one to use in particular situations?


https://codereview.chromium.org/633243002/

[dalec...@chromium.org]

Anything that a dev is likely to hit during development that could leave
them
frustratedly cursing at Chrome about should probably be logged to
media_log :)

Dealing with bug reports down the line would be much easier if the log has
easily read errors surfaced.

https://codereview.chromium.org/633243002/

[wole...@chromium.org]

On 2014/10/07 22:32:35, DaleCurtis wrote:
> Anything that a dev is likely to hit during development that could leave
> them
> frustratedly cursing at Chrome about should probably be logged to
> media_log :)

> Dealing with bug reports down the line would be much easier if the log has
> easily read errors surfaced.

+1000 to using MEDIA_LOG more for these cases.
Devs shouldn't have to resort to building Debug Chrome and running it with
various logging flags enabled just to find out why some media isn't playing
right.
It would also ease our triaging.


https://codereview.chromium.org/633243002/

[ser...@chromium.org]

Ok, changed to MEDIA_LOG. That's exactly why I want this added in the first
place. As I'm taking over media bug investigations for Chromecast I've had
more
than one opportunity to curse :)
Hopefully this will make logs a little more informative.

https://codereview.chromium.org/633243002/

[dalec...@chromium.org]

I defer to wolenetz@ for final lgtm, but you have mine :)

https://codereview.chromium.org/633243002/

[wole...@chromium.org]

https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demuxer.cc#newcode336
media/filters/chunk_demuxer.cc:336: << __FUNCTION__ << ": stream parsing
failed."
How does it look in chrome://media-internals when such a failure occurs?
Is inclusion of __FUNCTION__ too much detail?

https://codereview.chromium.org/633243002/

[ser...@chromium.org]

It would look like this:
00:00:47 37 error SourceState::Append: : stream parsing failed. Data
size=131072 append_window_start=1957.96 append_window_end=1960.92

I don't really care about function name inclusion. At Chromecast we usually
use
a system log instead of chrome://media-internals (Chromecast can only
display
pages, but navigating there is tricky due to lack of input devices), and
system
log already contains enough information (source file name + line number) to
figure out the source of the log message. So I'll leave it up to you.
We can also remove append_window info, if you'd prefer to make the message
shorter. I think in most cases we just want to know that parser returned an
error and maybe a data size for a minimal sanity check.

Please let me know what error message format you'd prefer.

https://codereview.chromium.org/633243002/

[wole...@chromium.org]

lgtm - This level of details seems good to me. Ship it!

https://codereview.chromium.org/633243002/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/633243002/20001


https://codereview.chromium.org/633243002/

[commi...@chromium.org]

Committed patchset #2 (id:20001) as 7237dc7e750089b5e9714371dd01b2cddb0314bb

https://codereview.chromium.org/633243002/

[commi...@chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/0e46f7a29a114db8079d6f2f9b9c594f0a496858
Cr-Commit-Position: refs/heads/master@{#298619}

https://codereview.chromium.org/633243002/