Emit Media Source codec names as media_log events.

Emit Media Source codec names as media_log events.

Media source codec names will appear in chrome://media-internals
and be recorded by the PipelineStatus histogram.

BUG=431447
Committed: https://crrev.com/14bcfc52eb12a1b08cca13198ba2b25a7156f2ce
Cr-Commit-Position: refs/heads/master@{#304943}

[watk, 2014-11-18 23:44:51]

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

[watk, 2014-11-18 23:44:51]

PTAL.

Question: In AudioCodecName I returned "pcm_s24le" for kSampleFormatS32 because
I copied this:
https://code.google.com/p/chromium/codesearch#chromium/src/media/ffmpeg/ffmpe...
. Is it a bug in ffmpeg_common.cc?

Tested by visiting http://dash-mse-test.appspot.com/dash-player.html and loading
various videos with chrome://media-internals open.
Observed the {video,audio}_codec_name and found_{audio,video}_stream properties
in the log, andq counts for PipelineStatus histograms.

[DaleCurtis, 2014-11-19 00:01:56]

Overall looks good! Effectively PCM_S24LE == PCM_S32LE and differentiating them
was hard, so we just use S32 in our sample format listing; so it's not a bug.

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
media/base/audio_decoder_config.cc:24: switch (sample_format) {
I wouldn't really worry about differentiating these, just saying pcm is fine.

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
media/base/audio_decoder_config.h:48: const char* AudioCodecName(AudioCodec
audio_codec, SampleFormat sample_format);
Instead of adding it like this, how about having a
AudioDecoderConfig::GetHumanReadableCodecName() function? The
AsHumanReadableString() method can then use this method too for better logs.

I'd also return std::string like that method too.

https://codereview.chromium.org/743483002/diff/60001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/video_decoder...
media/base/video_decoder_config.h:39: const char* VideoCodecName(VideoCodec
video_codec);
same suggestion as audio.

[DaleCurtis, 2014-11-19 00:03:57]

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

https://codereview.chromium.org/743483002/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:600: media_log_->SetStringProperty(
This works today since we don't allow codec switches in media source; wolenetz@
may have some ideas about verifying this here, but in the end I don't think it
really matters for the media-internals use case :)

[watk, 2014-11-19 19:18:09]

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
media/base/audio_decoder_config.cc:24: switch (sample_format) {
On 2014/11/19 00:01:56, DaleCurtis wrote:
> I wouldn't really worry about differentiating these, just saying pcm is fine.

Done.

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/audio_decoder...
media/base/audio_decoder_config.h:48: const char* AudioCodecName(AudioCodec
audio_codec, SampleFormat sample_format);
On 2014/11/19 00:01:56, DaleCurtis wrote:
> Instead of adding it like this, how about having a
> AudioDecoderConfig::GetHumanReadableCodecName() function? The
> AsHumanReadableString() method can then use this method too for better logs.
> 
> I'd also return std::string like that method too.

Done.

https://codereview.chromium.org/743483002/diff/60001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/743483002/diff/60001/media/base/video_decoder...
media/base/video_decoder_config.h:39: const char* VideoCodecName(VideoCodec
video_codec);
On 2014/11/19 00:01:56, DaleCurtis wrote:
> same suggestion as audio.

Done.

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

https://codereview.chromium.org/743483002/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:600: media_log_->SetStringProperty(
On 2014/11/19 00:03:57, DaleCurtis wrote:
> This works today since we don't allow codec switches in media source;
wolenetz@
> may have some ideas about verifying this here, but in the end I don't think it
> really matters for the media-internals use case :)

Good point. I did talk to wolenetz@ about this, and I believe he's working on
adding support for multiple SourceBuffers. What about always logging the
codec_name? The PipelineStatus values will only reflect the most recent codec,
but media-internals will display all of them.

[DaleCurtis, 2014-11-19 21:10:22]

https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
media/base/audio_decoder_config.cc:127: case kUnknownAudioCodec:
Combine with default: section below? Or remove default if it compiles, I think
we required switch statements to handle every case.

https://codereview.chromium.org/743483002/diff/100001/media/base/video_decode...
File media/base/video_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/base/video_decode...
media/base/video_decoder_config.cc:135: case kUnknownVideoCodec:
Ditto.

https://codereview.chromium.org/743483002/diff/100001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:612:
media_log_->SetStringProperty("audio_codec_name",
I worry this might end up creating a bunch of unnecessary events. How about only
logging this if it's the first audio stream or if
audio_->audio_decoder_config().codec() != audio_config.codec() ? Ditto for
video. Take a look at the accessors on ChunkDemuxerStream.

[watk, 2014-11-19 22:02:43]

https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
media/base/audio_decoder_config.cc:127: case kUnknownAudioCodec:
On 2014/11/19 21:10:22, DaleCurtis wrote:
> Combine with default: section below? Or remove default if it compiles, I think
> we required switch statements to handle every case.

Done. You're right, -Wswitch prevents it from compiling with missing cases.

https://codereview.chromium.org/743483002/diff/100001/media/base/video_decode...
File media/base/video_decoder_config.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/base/video_decode...
media/base/video_decoder_config.cc:135: case kUnknownVideoCodec:
On 2014/11/19 21:10:22, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/743483002/diff/100001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/743483002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:612:
media_log_->SetStringProperty("audio_codec_name",
On 2014/11/19 21:10:22, DaleCurtis wrote:
> I worry this might end up creating a bunch of unnecessary events. How about
only
> logging this if it's the first audio stream or if
> audio_->audio_decoder_config().codec() != audio_config.codec() ? Ditto for
> video. Take a look at the accessors on ChunkDemuxerStream.

Fair enough. I wasn't sure if the extra event per config change would be a
problem. Done.

[watk, 2014-11-19 22:39:24]

On 2014/11/19 22:02:43, watk wrote:
>
https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
> File media/base/audio_decoder_config.cc (right):
> 
>
https://codereview.chromium.org/743483002/diff/100001/media/base/audio_decode...
> media/base/audio_decoder_config.cc:127: case kUnknownAudioCodec:
> On 2014/11/19 21:10:22, DaleCurtis wrote:
> > Combine with default: section below? Or remove default if it compiles, I
think
> > we required switch statements to handle every case.
> 
> Done. You're right, -Wswitch prevents it from compiling with missing cases.

But now the android build has the error: "control reaches end of non-void
function" :( I added a return "" to satisfy the android build.

[DaleCurtis, 2014-11-19 23:29:45]

lgtm, nice work!

[watk, 2014-11-19 23:41:24]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2014-11-19 23:42:30]

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

[watk, 2014-11-19 23:48:06]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2014-11-20 00:29:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-20 00:31:07]

Patchset 7 (id:??) landed as
https://crrev.com/14bcfc52eb12a1b08cca13198ba2b25a7156f2ce
Cr-Commit-Position: refs/heads/master@{#304943}