media-internals: Differentiate error, info, and debug log messages

media-internals: Differentiate error, info, and debug log messages

This change refactors previous MediaLog::CreateMediaSourceErrorEvent()
to support general media-internals log messages that can be any one of
the new MediaLog::MediaLogLevel severities (just error, info, debug for
now).

Includes:
* Updated MEDIA_LOG and LIMITED_MEDIA_LOG macros are now additionally
  parameterized by ERROR, INFO, or DEBUG.
* Coalesced test LogCB implementations across
  source_buffer_stream_unittest, frame_processor_unittest, and
  chunk_demuxer_unittests into just one implementation in
  media/base/test_helpers.
* Differentiated the existing *MEDIA_LOG calls into the new severity
  buckets.

TEST=no regressions observed locally on linux (layout or
media_unittests), chrome://media-internals shows error, info, and debug
messages.

BUG=471902, 260005
Committed: https://crrev.com/6403f6c64482043ce1074ed8781a111dcd97ae3c
Cr-Commit-Position: refs/heads/master@{#323124}

[wolenetz, 2015-03-30 23:34:23]

wolenetz@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[wolenetz, 2015-03-30 23:36:02]

Dale, please take a look at patch set 2.
I also fixed a couple places in source_buffer_stream_unittest.cc where a dummy
LogCB() was being used on stream_ reset, though I don't think that warrants
mention in this CL's description.

Thanks!

[DaleCurtis, 2015-03-31 00:12:26]

Looks good, just a few questions.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc
File media/base/media_log.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:229: case MEDIALOG_ERROR:
Just use the MediaLogLevelToString() function instead of a new set of cases?

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:230: {
{ with : or did clang-format do this?

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h
File media/base/media_log.h (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h#...
media/base/media_log.h:104: #define MEDIA_LOG(log_cb, level) \
Hmm, we typically have the level before any other parameters with base::Logging.
What was the reason for point the level second?

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
File media/formats/mpeg/adts_stream_parser.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
media/formats/mpeg/adts_stream_parser.cc:72: MEDIA_LOG(log_cb(), ERROR) <<
"Invalid header data :" << std::hex
Hmm are these truly errors? They won't break Append() just cause it to skip the
frame.

https://codereview.chromium.org/1041353002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_service.cc:31:
media_log->AddEvent(media_log->CreateLogEvent(level, message));
Separately from this CL, I wonder if we should just have AddEvent(level,
message) instead of every location needing to call CreateLogEvent too.

[wolenetz, 2015-03-31 00:30:11]

Just some replies to comments (no updated code yet -- I will do that next):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc
File media/base/media_log.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:229: case MEDIALOG_ERROR:
On 2015/03/31 00:12:26, DaleCurtis wrote:
> Just use the MediaLogLevelToString() function instead of a new set of cases?

Yeah, I'll update that function to emit e.g. "error" instead of "MEDIALOG_ERROR"
and use it here.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:230: {
On 2015/03/31 00:12:26, DaleCurtis wrote:
> { with : or did clang-format do this?

I did this because otherwise event is declared multiple times in the same scope
and compiler errors.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h
File media/base/media_log.h (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h#...
media/base/media_log.h:104: #define MEDIA_LOG(log_cb, level) \
On 2015/03/31 00:12:26, DaleCurtis wrote:
> Hmm, we typically have the level before any other parameters with
base::Logging.
> What was the reason for point the level second?

No specific reason. I can change the sequence here in the macro. LogHelper's
.ctor: would it need to change, too?

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
File media/formats/mpeg/adts_stream_parser.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
media/formats/mpeg/adts_stream_parser.cc:72: MEDIA_LOG(log_cb(), ERROR) <<
"Invalid header data :" << std::hex
On 2015/03/31 00:12:26, DaleCurtis wrote:
> Hmm are these truly errors? They won't break Append() just cause it to skip
the
> frame.

You're right; not ERROR in all cases. I'll change to DEBUG.

https://codereview.chromium.org/1041353002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_service.cc:31:
media_log->AddEvent(media_log->CreateLogEvent(level, message));
On 2015/03/31 00:12:26, DaleCurtis wrote:
> Separately from this CL, I wonder if we should just have AddEvent(level,
> message) instead of every location needing to call CreateLogEvent too.

sgtm. I'll put something together after this CL to do that, though would
probably call it "AddLogEvent(level, message)" to reduce confusion with the
virtual AddEvent() interface that would need to remain (and that it would use).

[DaleCurtis, 2015-03-31 00:34:25]

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc
File media/base/media_log.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:230: {
On 2015/03/31 00:30:11, wolenetz wrote:
> On 2015/03/31 00:12:26, DaleCurtis wrote:
> > { with : or did clang-format do this?
> 
> I did this because otherwise event is declared multiple times in the same
scope
> and compiler errors.

I just meant the style, { should be on the same line as : unless clang-format
did this.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h
File media/base/media_log.h (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h#...
media/base/media_log.h:104: #define MEDIA_LOG(log_cb, level) \
On 2015/03/31 00:30:11, wolenetz wrote:
> On 2015/03/31 00:12:26, DaleCurtis wrote:
> > Hmm, we typically have the level before any other parameters with
> base::Logging.
> > What was the reason for point the level second?
> 
> No specific reason. I can change the sequence here in the macro. LogHelper's
> .ctor: would it need to change, too?

I think the constructor can stay the same if you decide to change this. I prefer
it with level first, but if I'm the minority it's fine as is.

[wolenetz, 2015-03-31 19:20:22]

Please take a look at patch set 4.
Thanks.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc
File media/base/media_log.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:229: case MEDIALOG_ERROR:
On 2015/03/31 00:30:11, wolenetz wrote:
> On 2015/03/31 00:12:26, DaleCurtis wrote:
> > Just use the MediaLogLevelToString() function instead of a new set of cases?
> 
> Yeah, I'll update that function to emit e.g. "error" instead of
"MEDIALOG_ERROR"
> and use it here.

Done.

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.cc...
media/base/media_log.cc:230: {
On 2015/03/31 00:34:25, DaleCurtis wrote:
> On 2015/03/31 00:30:11, wolenetz wrote:
> > On 2015/03/31 00:12:26, DaleCurtis wrote:
> > > { with : or did clang-format do this?
> > 
> > I did this because otherwise event is declared multiple times in the same
> scope
> > and compiler errors.
> 
> I just meant the style, { should be on the same line as : unless clang-format
> did this.

I've now git-cl-format'ed the whole thing. It was my bad style previously. I've
also streamlined this method to use a new helper MediaLogLevelToEventType().

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h
File media/base/media_log.h (right):

https://codereview.chromium.org/1041353002/diff/20001/media/base/media_log.h#...
media/base/media_log.h:104: #define MEDIA_LOG(log_cb, level) \
On 2015/03/31 00:34:25, DaleCurtis wrote:
> On 2015/03/31 00:30:11, wolenetz wrote:
> > On 2015/03/31 00:12:26, DaleCurtis wrote:
> > > Hmm, we typically have the level before any other parameters with
> > base::Logging.
> > > What was the reason for point the level second?
> > 
> > No specific reason. I can change the sequence here in the macro. LogHelper's
> > .ctor: would it need to change, too?
> 
> I think the constructor can stay the same if you decide to change this. I
prefer
> it with level first, but if I'm the minority it's fine as is.

Done (level first for both the macros and the .ctor). Thanks!

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
File media/formats/mpeg/adts_stream_parser.cc (right):

https://codereview.chromium.org/1041353002/diff/20001/media/formats/mpeg/adts...
media/formats/mpeg/adts_stream_parser.cc:72: MEDIA_LOG(log_cb(), ERROR) <<
"Invalid header data :" << std::hex
On 2015/03/31 00:30:11, wolenetz wrote:
> On 2015/03/31 00:12:26, DaleCurtis wrote:
> > Hmm are these truly errors? They won't break Append() just cause it to skip
> the
> > frame.
> 
> You're right; not ERROR in all cases. I'll change to DEBUG.

Done.

[DaleCurtis, 2015-03-31 20:46:17]

lgtm % up-to-you nit.

https://codereview.chromium.org/1041353002/diff/60001/media/base/media_log.cc
File media/base/media_log.cc (right):

https://codereview.chromium.org/1041353002/diff/60001/media/base/media_log.cc...
media/base/media_log.cc:241: const MediaLogEvent::Type type =
MediaLogLevelToEventType(level);
Just inline these two variables?

[wolenetz, 2015-03-31 21:01:39]

On 2015/03/31 20:46:17, DaleCurtis wrote:
> lgtm % up-to-you nit.
> 
> https://codereview.chromium.org/1041353002/diff/60001/media/base/media_log.cc
> File media/base/media_log.cc (right):
> 
>
https://codereview.chromium.org/1041353002/diff/60001/media/base/media_log.cc...
> media/base/media_log.cc:241: const MediaLogEvent::Type type =
> MediaLogLevelToEventType(level);
> Just inline these two variables?

Yeah, I saw this while working on the next CL
(https://codereview.chromium.org/1047313002/). I'll keep it in that one and not
here.

Thanks for reviews!

[wolenetz, 2015-03-31 21:07:48]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2015-03-31 21:10:04]

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

[commit-bot: I haz the power, 2015-03-31 22:05:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-31 22:07:03]

Patchset 4 (id:??) landed as
https://crrev.com/6403f6c64482043ce1074ed8781a111dcd97ae3c
Cr-Commit-Position: refs/heads/master@{#323124}