MSE: Verify MediaLog events created by existing WebM unit tests

MSE: Verify MediaLog events created by existing WebM unit tests

Similar to previous SourceBufferStreamTests update in r342861, this
change updates media unit tests to provide a StrictMock<MockMediaLog> to
verify MediaLog events produced with existing MSE WebM unit tests. While
log message maintenance burden is increased, helpers are included to
reduce that burden while gaining improved resilience against logging
regression.

This CL does not verify all possible MSE WebM parser MEDIA_LOGs, just
those that are emitted by existing unit tests.

TEST=no regression

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

[wolenetz, 2015-08-18 21:11:34]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 21:12:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1300943002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1300943002/1

[wolenetz, 2015-08-18 21:13:48]

wolenetz@chromium.org changed reviewers:
+ chcunningham@chromium.org, xhwang@chromium.org

[wolenetz, 2015-08-18 21:13:49]

Please review patch set 1:
xhwang@: Everything (committer), especially webm_content_encodings_*.
chcunningham@: Everything, especially the duration estimation log.
Thanks!

https://codereview.chromium.org/1300943002/diff/1/media/base/mock_media_log.h
File media/base/mock_media_log.h (right):

https://codereview.chromium.org/1300943002/diff/1/media/base/mock_media_log.h...
media/base/mock_media_log.h:14: #define EXPECT_MEDIA_LOG_STRING(x) \
I lifted this helper macro out of SourceBufferStreamTests, since it's useful now
in other places.

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
media/formats/webm/webm_cluster_parser_unittest.cc:65: const int kTimecodeScale
= 1000000;
This is just drive-by opportunistic style correction ;)

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
media/formats/webm/webm_cluster_parser_unittest.cc:317: // Helper that
hard-codes some non-varying constructor parameters.
I moved these to be instance methods so they have access to |media_log_| without
adding yet another parameter to each of these functions.

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
media/formats/webm/webm_tracks_parser_unittest.cc:30: void
VerifyTextTrackInfo(const uint8* buffer,
ditto: I moved this to be an instance method to give access to |media_log_|
without adding parameter to function.

[commit-bot: I haz the power, 2015-08-18 22:40:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 22:40:45]

Dry run: This issue passed the CQ dry run.

[xhwang, 2015-08-19 00:25:06]

Looking good. Just have some questions and nits.

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
media/formats/webm/webm_cluster_parser_unittest.cc:396: const int
kExpectedLastVideoBufferEstimatedDurationInMs = 33;
I am a bit lost here. Why this one is special and needs a constant while we
still use hardcoded numbers for others? Same question for 
kExpectedLastAudioBufferEstimatedDurationInMs and
kExpectedLastVideoBufferEstimatedDurationInMs below.

Also, the variable name is really long :(

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_con...
File media/formats/webm/webm_content_encodings_client_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_con...
media/formats/webm/webm_content_encodings_client_unittest.cc:74:
EXPECT_MEDIA_LOG_STRING(MissingContentEncoding());
nit: hmm, can we just use EXPECT_MEDIA_LOG() which is shorter?

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
media/formats/webm/webm_tracks_parser_unittest.cc:196: HasSubstr("Illegal 0ns
audio TrackEntry DefaultDuration"));
Do we really have "ns" precision in our code? 1ns is 0.001 microsecond....

[xhwang, 2015-08-19 00:25:07]

Looking good. Just have some questions and nits.

[chcunningham, 2015-08-19 19:57:23]

lgtm % xhwangs questions. None of my own to add.

[wolenetz, 2015-08-24 19:33:51]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[wolenetz, 2015-08-24 19:34:03]

Please take a look at patch set 3. Thanks!

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_clu...
media/formats/webm/webm_cluster_parser_unittest.cc:396: const int
kExpectedLastVideoBufferEstimatedDurationInMs = 33;
On 2015/08/19 00:25:05, xhwang wrote:
> I am a bit lost here. Why this one is special and needs a constant while we
> still use hardcoded numbers for others? Same question for 
> kExpectedLastAudioBufferEstimatedDurationInMs and
> kExpectedLastVideoBufferEstimatedDurationInMs below.
> 
> Also, the variable name is really long :(

I'll simplify the name. All 33ms hard-code for kVideoTrackNum frame duration
verification in kBlockInfo[] could be replaced with this identifier, but I
wanted to make it clear which frame, exactly, this 33ms estimate was to be
determined for, hence I used it only for the last video track block in
kBlockInfo[]. The previous video blocks' durations are derived from inter-block
timestamp deltas. This last block's duration is estimated based on previously
seen durations (which in this case, but not all the ones in this file, all are
33).

Since this CL adds media log string verification of the estimation calculation,
the last (video track in this case) buffer's duration expectation from
kBlockInfo is verified in two places now, so I added a named constant to help
future maintenance/readability of this test.

This same pattern is adhered to (sometimes for audio track) in other updated
tests in this CL.

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_con...
File media/formats/webm/webm_content_encodings_client_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_con...
media/formats/webm/webm_content_encodings_client_unittest.cc:74:
EXPECT_MEDIA_LOG_STRING(MissingContentEncoding());
On 2015/08/19 00:25:05, xhwang wrote:
> nit: hmm, can we just use EXPECT_MEDIA_LOG() which is shorter?

SGTM. Done.
In this CL, I also responded to dependent CL's comment by chcunningham@
(https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...)
and have moved CONTAINS_STRING macro definition into mock_media_log. Different
redefinition of macros causes precompilation errors, so if somehow
"CONTAINS_STRING" becomes redefined elsewhere later in scope of the same
compilation target, that will cause compile error. Before I confirmed this, I
was uncomfortable putting such a generically named macro into a wider scope,
even if test-only.

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_tra...
media/formats/webm/webm_tracks_parser_unittest.cc:196: HasSubstr("Illegal 0ns
audio TrackEntry DefaultDuration"));
On 2015/08/19 00:25:05, xhwang wrote:
> Do we really have "ns" precision in our code? 1ns is 0.001 microsecond....

tl;dr: yes :)

Detail:
WebM container (derived from a stricter subset of an earlier Matroska spec)
defines a TrackEntry DefaultDuration (and some others) in integer nanoseconds.
This is unlike BlockDuration (and yet some others) which is scaled by
TimeCodeScale.

Yet more detail:
A value of 0 is illegal for TrackEntry DefaultDuration (matroska spec indicates
the valid range for this unsigned integer field is "not 0", if the field is
included).

references:
webm: http://www.webmproject.org/docs/container/
matroska: http://www.matroska.org/technical/specs/index.html

[commit-bot: I haz the power, 2015-08-24 19:34:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1300943002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1300943002/40001

[commit-bot: I haz the power, 2015-08-24 20:52:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-24 20:52:53]

Dry run: This issue passed the CQ dry run.

[xhwang, 2015-08-24 21:14:13]

lgtm

[wolenetz, 2015-08-24 21:19:15]

The CQ bit was checked by wolenetz@chromium.org

[wolenetz, 2015-08-24 21:19:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/1300943002/#ps40001
(title: "Addresses xhwang@'s comments")

[commit-bot: I haz the power, 2015-08-24 21:19:24]

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

[commit-bot: I haz the power, 2015-08-24 21:24:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-24 21:25:21]

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

[wolenetz, 2015-08-24 23:37:02]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1310423003/ by wolenetz@chromium.org.

The reason for reverting is: This is breaking windows bots
(http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28....

[wolenetz, 2015-08-25 19:21:30]

On 2015/08/24 23:37:02, wolenetz wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/1310423003/ by mailto:wolenetz@chromium.org.
> 
> The reason for reverting is: This is breaking windows bots
>
(http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28....

Reland attempt of a fixed version of this CL is at
https://codereview.chromium.org/1306923004/