MSE: Verify MediaLog events created by existing MP4 unit tests

MSE: Verify MediaLog events created by existing MP4 unit tests

Similar to previous changes r342861 and
https://codereview.chromium.org/1300943002/, this change updates media
unit tests to provide a StrictMock<MockMediaLog> to verify MediaLog
events produced with existing MSE MP4 unit tests. While log message
maintenance burden is increased, helpers are used to reduce that burden
while gaining improved resilience against logging regression.

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

TEST=no regression

Committed: https://crrev.com/194a2651ece516dc91fa0da9aa1675afd307e36e
Cr-Commit-Position: refs/heads/master@{#345183}

[wolenetz, 2015-08-18 22:46:39]

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

[commit-bot: I haz the power, 2015-08-18 22:47:15]

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

[wolenetz, 2015-08-18 22:50:05]

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

[wolenetz, 2015-08-18 22:50:06]

Note: Landing this depends on first landing
https://codereview.chromium.org/1300943002/.

Please review patch set 1:
xhwang@: Everything (committer).
chcunningham@: Everything (MSE).
Thanks!

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/box_reade...
File media/formats/mp4/box_reader_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/box_reade...
media/formats/mp4/box_reader_unittest.cc:92: void TestTopLevelBox(const uint8*
data, int size, uint32 fourCC) {
Note, I moved this to be an instance method, so it has access to |media_log_|
without adding a parameter to the function.

[xhwang, 2015-08-18 23:16:31]

lgtm

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

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

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

Dry run: This issue passed the CQ dry run.

[chcunningham, 2015-08-19 19:36:19]

lgtm % nits

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
File media/formats/mp4/aac_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
media/formats/mp4/aac_unittest.cc:19: #define CONTAINS_STRING(arg, x)
(std::string::npos != (arg).find(x))
We define this 3 places now (including sbs tests). You could put it in
mock_media_log.h?

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
media/formats/mp4/aac_unittest.cc:169:
EXPECT_MEDIA_LOG_STRING(AudioCodecLog("mp4a.40.1")).Times(2);
Do you know why we do it twice here? Not your fault, but we really should have
put comments on the magic byte values for these tests :(.

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
media/formats/mp4/mp4_stream_parser_unittest.cc:247: InSequence s;
Do you need this? Ditto for line 279.

[chcunningham, 2015-08-19 20:06:44]

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
media/formats/mp4/mp4_stream_parser_unittest.cc:247: InSequence s;
On 2015/08/19 19:36:18, chcunningham wrote:
> Do you need this? Ditto for line 279.

ignore... I read up on InSequence. Kinda whacky, but I get it

[wolenetz, 2015-08-24 20: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-24 20:12:22]

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

[wolenetz, 2015-08-24 20:13:52]

Thanks for reviews. Currently, patch set 2 is pending landing prereq CL first.

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
File media/formats/mp4/aac_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
media/formats/mp4/aac_unittest.cc:19: #define CONTAINS_STRING(arg, x)
(std::string::npos != (arg).find(x))
On 2015/08/19 19:36:18, chcunningham wrote:
> We define this 3 places now (including sbs tests). You could put it in
> mock_media_log.h?

Done (here and in prerequisite CL, since it had some definitions of this, too).
See
https://codereview.chromium.org/1300943002/diff/1/media/formats/webm/webm_con...
Thanks for pointing this out...

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/aac_unitt...
media/formats/mp4/aac_unittest.cc:169:
EXPECT_MEDIA_LOG_STRING(AudioCodecLog("mp4a.40.1")).Times(2);
On 2015/08/19 19:36:18, chcunningham wrote:
> Do you know why we do it twice here? Not your fault, but we really should have
> put comments on the magic byte values for these tests :(. 

The intrinsic parse sequence emits the AudioCodecLog before fully successful
parse. So both the EXPECT_FALSE and EXPECT_TRUE lines, below, will cause this
log to be emitted. Since it's a StrictMock'ed MockMediaLog, we need to expect
both for this test to pass with current production code.

Cleaning up both this test (magic byte code numbers) and the parser to perhaps
be less optimistic, and more detailed, in its logging is, I think, out of scope
of this CL. I've filed a P3 to work on this though: https://crbug.com/524159

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1300013002/diff/1/media/formats/mp4/mp4_strea...
media/formats/mp4/mp4_stream_parser_unittest.cc:247: InSequence s;
On 2015/08/19 20:06:43, chcunningham wrote:
> On 2015/08/19 19:36:18, chcunningham wrote:
> > Do you need this? Ditto for line 279.
> 
> ignore... I read up on InSequence. Kinda whacky, but I get it

Acknowledged.

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

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

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

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by wolenetz@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1300013002/#ps20001
(title: "Rebased and addressed nits. Pending landing prereq before sending to
CQ.")

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

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

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

Committed patchset #2 (id:20001)

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

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

[wolenetz, 2015-08-24 23:34:40]

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

The reason for reverting is: Prerequisite CL
(https://codereview.chromium.org/1300943002/) is breaking windows bots
(http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...),
so reverting both this and it to unblock tree..