Log stream parsing errors in ChunkDemuxer

Log stream parsing errors in ChunkDemuxer

BUG=

Committed: https://crrev.com/0e46f7a29a114db8079d6f2f9b9c594f0a496858
Cr-Commit-Position: refs/heads/master@{#298619}

[servolk, 2014-10-07 21:26:38]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, vrk@chromium.org, wolenetz@chromium.org

[servolk, 2014-10-07 21:54:02]

PTAL

[DaleCurtis, 2014-10-07 22:08:16]

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....
media/filters/chunk_demuxer.cc:335: DVLOG(1) << __FUNCTION__ << ": stream
parsing failed."
Is this something better sent to MEDIA_LOG() ?

[servolk, 2014-10-07 22:20:52]

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....
> 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?

[DaleCurtis, 2014-10-07 22:32:35]

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.

[wolenetz, 2014-10-07 22:39:44]

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.

[servolk, 2014-10-07 22:45:59]

On 2014/10/07 22:39:44, wolenetz wrote:
> 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.

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.

[DaleCurtis, 2014-10-07 22:49:51]

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

[wolenetz, 2014-10-07 22:50:48]

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

https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demu...
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?

[servolk, 2014-10-07 23:07:14]

On 2014/10/07 22:50:48, wolenetz wrote:
>
https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demu...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demu...
> 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?

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.

[wolenetz, 2014-10-07 23:21:00]

On 2014/10/07 23:07:14, servolk wrote:
> On 2014/10/07 22:50:48, wolenetz wrote:
> >
>
https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demu...
> > File media/filters/chunk_demuxer.cc (right):
> > 
> >
>
https://codereview.chromium.org/633243002/diff/20001/media/filters/chunk_demu...
> > 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?
> 
> 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.

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

[servolk, 2014-10-07 23:24:43]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2014-10-07 23:25:57]

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

[commit-bot: I haz the power, 2014-10-08 00:35:28]

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

[commit-bot: I haz the power, 2014-10-08 00:36:24]

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