Improve logging output for DecoderBuffer and VideoFrame

Improve logging output for DecoderBuffer and VideoFrame

Helps show a summary of the objects when logging enabled.

BUG=none

Committed: https://crrev.com/b60311d3307f0b3f8267f09f51bfcd17070ab6c2
Cr-Commit-Position: refs/heads/master@{#369846}

[jrummell, 2015-12-18 01:22:21]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2015-12-18 01:22:21]

PTAL.

[xhwang, 2015-12-18 23:07:44]

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/base/decoder_b...
File media/base/decoder_buffer.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/base/decoder_b...
media/base/decoder_buffer.cc:104: << base::HexEncode(data_.get() + size_ - 16,
16);
I am not sure whether we want to log this by default. In very rare cases do we
actually care about the bytes in the buffer...

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/base/video_fra...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/base/video_fra...
media/base/video_frame.cc:867: s << row_bytes(i) << " ";
rows and strides are no as important as other fields. We can probably drop them
by default, or have a "bool verbose" parameter in this function and only display
then when "verbose" is true..

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/cdm/cdm_adapte...
File media/cdm/cdm_adapter.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/cdm/cdm_adapte...
media/cdm/cdm_adapter.cc:665: << " encrypted: " <<
encrypted->AsHumanReadableString();
I like AsHumanReadableString(). But I don't feel we should enable it here by
default. It will generate too much noise that could hide real useful logs...

https://chromiumcodereview.appspot.com/1536783003/diff/1/media/cdm/cdm_adapte...
media/cdm/cdm_adapter.cc:688: << " decoded_frame: " <<
decoded_frame->AsHumanReadableString();
ditto

[jrummell, 2015-12-22 01:30:43]

Updated (with rebase). I didn't realize that somebody had changed uint8 to
uint8_t in the files, or else I wouldn't have done a rebase.

https://codereview.chromium.org/1536783003/diff/1/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/1536783003/diff/1/media/base/decoder_buffer.c...
media/base/decoder_buffer.cc:104: << base::HexEncode(data_.get() + size_ - 16,
16);
On 2015/12/18 23:07:43, xhwang wrote:
> I am not sure whether we want to log this by default. In very rare cases do we
> actually care about the bytes in the buffer...

Reduced the string to just 35 characters, so it won't be much extra data logged.

https://codereview.chromium.org/1536783003/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1536783003/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:867: s << row_bytes(i) << " ";
On 2015/12/18 23:07:43, xhwang wrote:
> rows and strides are no as important as other fields. We can probably drop
them
> by default, or have a "bool verbose" parameter in this function and only
display
> then when "verbose" is true..

Removed.

https://codereview.chromium.org/1536783003/diff/1/media/cdm/cdm_adapter.cc
File media/cdm/cdm_adapter.cc (right):

https://codereview.chromium.org/1536783003/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:665: << " encrypted: " <<
encrypted->AsHumanReadableString();
On 2015/12/18 23:07:43, xhwang wrote:
> I like AsHumanReadableString(). But I don't feel we should enable it here by
> default. It will generate too much noise that could hide real useful logs...

Changed to DVLOG(5), so you only see it if you enable everything.

[xhwang, 2016-01-05 17:29:11]

Looking good mostly. Just some more discussions about what we want to log. Sorry
for the delay.

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/base/decod...
File media/base/decoder_buffer.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/base/decod...
media/base/decoder_buffer.cc:102: << base::HexEncode(data_.get() + size_ - 8,
8);
I still don't see a general use case for dumping the binary data. Note that
AsHumanReadableString() is a generic function and can be used in a lot of
places, most of which won't be interested in the binary data. 

When in the rare cases where we are actually interested, we can always manually
dump the binary data.

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/base/video...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/base/video...
media/base/video_frame.cc:74: return "??";
Maybe return something like "INVALID" instead of "??"?

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/cdm/cdm_ad...
File media/cdm/cdm_adapter.cc (right):

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter.cc:664: DVLOG(5) << __FUNCTION__
Move this to the beginning of this function?

Also, DVLOG(3) should be good here, if we don't log the binary data (see comment
above).

https://chromiumcodereview.appspot.com/1536783003/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter.cc:688: << " decoded_frame: " <<
decoded_frame->AsHumanReadableString();
We have only 5 cases of DVLOG(5) in the entire code base, which makes me feel we
shouldn't use DVLOG(5) for logs that are less useful. In this particular case,
this log is only useful when we have bugs in converting VideoFrames. After we
fix the bug, we probably never need this log anymore. So instead of having a
DVLOG(5) here, maybe we should only add it temporarily during debug when we need
it?

[jrummell, 2016-01-12 20:00:37]

Updated.

https://codereview.chromium.org/1536783003/diff/20001/media/base/decoder_buff...
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/1536783003/diff/20001/media/base/decoder_buff...
media/base/decoder_buffer.cc:102: << base::HexEncode(data_.get() + size_ - 8,
8);
On 2016/01/05 17:29:11, xhwang wrote:
> I still don't see a general use case for dumping the binary data. Note that
> AsHumanReadableString() is a generic function and can be used in a lot of
> places, most of which won't be interested in the binary data. 
> 
> When in the rare cases where we are actually interested, we can always
manually
> dump the binary data.

Done.

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

https://codereview.chromium.org/1536783003/diff/20001/media/base/video_frame....
media/base/video_frame.cc:74: return "??";
On 2016/01/05 17:29:11, xhwang wrote:
> Maybe return something like "INVALID" instead of "??"?

Done.

https://codereview.chromium.org/1536783003/diff/20001/media/cdm/cdm_adapter.cc
File media/cdm/cdm_adapter.cc (right):

https://codereview.chromium.org/1536783003/diff/20001/media/cdm/cdm_adapter.c...
media/cdm/cdm_adapter.cc:664: DVLOG(5) << __FUNCTION__
On 2016/01/05 17:29:11, xhwang wrote:
> Move this to the beginning of this function?
> 
> Also, DVLOG(3) should be good here, if we don't log the binary data (see
comment
> above).

Done.

https://codereview.chromium.org/1536783003/diff/20001/media/cdm/cdm_adapter.c...
media/cdm/cdm_adapter.cc:688: << " decoded_frame: " <<
decoded_frame->AsHumanReadableString();
On 2016/01/05 17:29:11, xhwang wrote:
> We have only 5 cases of DVLOG(5) in the entire code base, which makes me feel
we
> shouldn't use DVLOG(5) for logs that are less useful. In this particular case,
> this log is only useful when we have bugs in converting VideoFrames. After we
> fix the bug, we probably never need this log anymore. So instead of having a
> DVLOG(5) here, maybe we should only add it temporarily during debug when we
need
> it? 

Done.

[xhwang, 2016-01-15 08:25:59]

lgtm

[jrummell, 2016-01-15 20:11:47]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-01-15 20:11:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1536783003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-15 20:12:43]

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

[commit-bot: I haz the power, 2016-01-15 21:37:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-15 21:39:20]

Description was changed from

==========
Improve logging output for DecoderBuffer and VideoFrame

Helps show a summary of the objects when logging enabled.

BUG=none
==========

to

==========
Improve logging output for DecoderBuffer and VideoFrame

Helps show a summary of the objects when logging enabled.

BUG=none

Committed: https://crrev.com/b60311d3307f0b3f8267f09f51bfcd17070ab6c2
Cr-Commit-Position: refs/heads/master@{#369846}
==========

[commit-bot: I haz the power, 2016-01-15 21:39:21]

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