Support MPEG1 audio in the MPEG2-TS stream parser.

Support MPEG1 audio in the MPEG2-TS stream parser.

BUG=None

Committed: https://crrev.com/19d61295f56988233119c544edeee25f4322a17f
Cr-Commit-Position: refs/heads/master@{#294737}

[damienv1, 2014-08-26 17:28:49]

damienv@chromium.org changed reviewers:
+ acolwell@chromium.org, wolenetz@chromium.org

[damienv1, 2014-08-26 17:33:55]

This is mostly for information that I plan to add Mpeg1 audio support for
Mpeg2TS.
So currently, no need to have a deep look at it.

[DaleCurtis, 2014-08-26 18:54:40]

I defer to acolwell@ on whether we want this, but if we do it'd be nice to reuse
the MPEGAudioStreamParserBase instead of copying code.

[damienv1, 2014-08-26 19:21:05]

Totally agree. The first patch is just a working patch just for a quick test (no
unit test, no code refactoring).
I plan to reuse to code from Mp3StreamParser (most likely by extracting the
common features)

[wolenetz, 2014-08-26 20:26:31]

+1 to other comments. Superficial nits otherwise; deep review not done.

https://codereview.chromium.org/506943003/diff/1/media/formats/mp2t/es_parser...
File media/formats/mp2t/es_parser_mpeg1audio.cc (right):

https://codereview.chromium.org/506943003/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_mpeg1audio.cc:24: // are allowed.
nit: describe columns and indices? (in case the referenced webpage becomes out
of sync with this)

https://codereview.chromium.org/506943003/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_mpeg1audio.cc:26: static const bool
kIsAllowed[17][4] = {
nit: s/17/16/ ?

[damienv1, 2014-09-03 17:43:43]

Please take another look. Note: the unit test is still missing (will do as part
of another patch set).
Thanks.

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
File media/formats/mpeg/mp3_stream_parser.cc (right):

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
media/formats/mpeg/mp3_stream_parser.cc:10: const int
MP3StreamParser::kMpeg1AudioHeaderSize = 4u;
Should be 4 (not 4u)

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
File media/formats/mpeg/mp3_stream_parser.h (right):

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
media/formats/mpeg/mp3_stream_parser.h:29: // audio header (i.e. should be
greater than or equal to 4).
Update the comment using kMpeg1AudioHeaderSize instead of hard-coded 4.

[damienv1, 2014-09-03 17:43:55]

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

[damienv1, 2014-09-03 18:59:54]

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
File media/formats/mpeg/mp3_stream_parser.h (right):

https://codereview.chromium.org/506943003/diff/20001/media/formats/mpeg/mp3_s...
media/formats/mpeg/mp3_stream_parser.h:30: static bool
ParserMpeg1AudioFrameHeader(
Could be simply ParseMpeg1AudioHeader.

[DaleCurtis, 2014-09-03 22:04:40]

You mention MPEG1 everywhere, but the MP3 parser supports v1,v2,v3. Is that what
you want?

[damienv1, 2014-09-03 22:11:29]

v1, v2 and v3 refers to layers.
MP3 is known as MPEG-1 audio layer3.
So MP3StreamParser is not really a correct name.

Where I agree is that Mpeg1 is too restrictive since Mpeg1 Mpeg2 and Mpeg2.5
audio is supported.
So I can rename it to simply MpegAudio instead of Mpeg1Audio.

[DaleCurtis, 2014-09-03 22:31:28]

Sorry the v3 was a typo, I did mean v1,v2,v2.5 like you indicate.

It's (believe) it's named mp3_stream_parser since that's the typical container,
acolwell can confirm. We already have a MPEGAudioStreamParserBase. I don't think
it's a big deal to rename MP3StreamParser to MPEGAudioStreamParser, but again
defer to acolwell@.

[acolwell GONE FROM CHROMIUM, 2014-09-03 23:30:45]

On 2014/09/03 22:31:28, DaleCurtis wrote:
> Sorry the v3 was a typo, I did mean v1,v2,v2.5 like you indicate.
> 
> It's (believe) it's named mp3_stream_parser since that's the typical
container,
> acolwell can confirm. We already have a MPEGAudioStreamParserBase. I don't
think
> it's a big deal to rename MP3StreamParser to MPEGAudioStreamParser, but again
> defer to acolwell@.

I agree with Damien. MPEG Audio is the proper term to use. MP3 was simply used
as a container shorthand, but since code is being shared now, I think it is more
appropriate to use MPEG audio instead.

[damienv1, 2014-09-04 18:46:35]

Please take a look. Thanks !

[acolwell GONE FROM CHROMIUM, 2014-09-04 22:20:53]

lgtm % nits

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio.cc (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:43: bool
EsParserMpeg1Audio::LookForMpeg1AudioFrame(
nit: Move this and the skip method to a later point in the file so they roughly
match the decl order.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:68: es_queue_->Pop(offset);
nit: You might want to add some text here indicating that the Pop() is dropping
garbage data that is appearing between frames.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:127:
audio_timestamp_helper_->SetBaseTimestamp(current_timing_desc.pts);
It seems like this could introduce drift. Shouldn't this be in the block below
so that we only set the base on the first frame we know the timestamp for. It
seems like only if there is a large gap between pts and what the
audio_timestamps_helper says should we rebase the helper.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:173: LogCB log_cb =
base::Bind(&DummyMediaLog);
nit: You should probably just make this a member variable and only bind in the
constructor since that is likely what the code would need to look like if you
had the logging in place.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
File media/formats/mpeg/mpeg1_audio_stream_parser.cc (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:10: const int
MPEG1AudioStreamParser::kHeaderSize = 4u;
nit: drop the u?

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:248: if (size <
static_cast<int>(kHeaderSize))
I don't think you need the cast here since kHeaderSize is an int.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
File media/formats/mpeg/mpeg1_audio_stream_parser.h (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.h:21: int version;
nit: Since this is a public struct, version and layer should probably be enums
that contain the kVersionXX and kLayerXX values so that other code that uses
this struct doesn't need its own private constants.

[DaleCurtis, 2014-09-04 22:42:40]

I'm confused why you are still calling everything MPEG1, I thought we agreed
above to change this to MPEGAudioStreamParser everywhere?

[acolwell GONE FROM CHROMIUM, 2014-09-04 22:45:44]

On 2014/09/04 22:42:40, DaleCurtis wrote:
> I'm confused why you are still calling everything MPEG1, I thought we agreed
> above to change this to MPEGAudioStreamParser everywhere?

Damien and I had an IM conversation yesterday about this. MPEG1 is a better name
than MPEG audio because technically AAC is MPEG audio as well. The MPEG1 spec is
what actually introduced the tools and modes that is typically referred to as
MP3. MPEG2 & 2.5 really only change the sample rates and don't actually add any
new tools. AAC is a different story and actually is a completely different
coding system and so it got its own name.

[DaleCurtis, 2014-09-04 23:01:14]

I defer on the ES specifics to acolwell, the mpeg1 changes lgtm

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
File media/formats/mpeg/mpeg1_audio_stream_parser.cc (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:10: const int
MPEG1AudioStreamParser::kHeaderSize = 4u;
Just make this an enum?

[damienv1, 2014-09-05 20:06:13]

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio.cc (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:43: bool
EsParserMpeg1Audio::LookForMpeg1AudioFrame(
On 2014/09/04 22:20:53, acolwell wrote:
> nit: Move this and the skip method to a later point in the file so they
roughly
> match the decl order.

Done.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:68: es_queue_->Pop(offset);
On 2014/09/04 22:20:53, acolwell wrote:
> nit: You might want to add some text here indicating that the Pop() is
dropping
> garbage data that is appearing between frames.

Done.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:127:
audio_timestamp_helper_->SetBaseTimestamp(current_timing_desc.pts);
On 2014/09/04 22:20:53, acolwell wrote:
> It seems like this could introduce drift. Shouldn't this be in the block below
> so that we only set the base on the first frame we know the timestamp for. It
> seems like only if there is a large gap between pts and what the
> audio_timestamps_helper says should we rebase the helper.

I think the parser cannot do any assumption about the way data was muxed.
The parser does its best when timestamps are not available. However, when
timestamps
are available in the stream, they should be trusted.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:173: LogCB log_cb =
base::Bind(&DummyMediaLog);
On 2014/09/04 22:20:53, acolwell wrote:
> nit: You should probably just make this a member variable and only bind in the
> constructor since that is likely what the code would need to look like if you
> had the logging in place.

Seems to be a good idea :)

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
File media/formats/mpeg/mpeg1_audio_stream_parser.cc (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:10: const int
MPEG1AudioStreamParser::kHeaderSize = 4u;
On 2014/09/04 22:20:53, acolwell wrote:
> nit: drop the u?

Done.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:10: const int
MPEG1AudioStreamParser::kHeaderSize = 4u;
On 2014/09/04 23:01:14, DaleCurtis wrote:
> Just make this an enum?

Done.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.cc:248: if (size <
static_cast<int>(kHeaderSize))
On 2014/09/04 22:20:53, acolwell wrote:
> I don't think you need the cast here since kHeaderSize is an int.

Done.

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
File media/formats/mpeg/mpeg1_audio_stream_parser.h (right):

https://codereview.chromium.org/506943003/diff/60001/media/formats/mpeg/mpeg1...
media/formats/mpeg/mpeg1_audio_stream_parser.h:21: int version;
On 2014/09/04 22:20:53, acolwell wrote:
> nit: Since this is a public struct, version and layer should probably be enums
> that contain the kVersionXX and kLayerXX values so that other code that uses
> this struct doesn't need its own private constants.

Done.

[damienv1, 2014-09-11 15:05:28]

Matt, any more feedback ?
Thanks.

[wolenetz, 2014-09-11 17:43:31]

On 2014/09/11 15:05:28, damienv1 wrote:
> Matt, any more feedback ?
> Thanks.

My apologies - I'll do a full pass on this one today.

[wolenetz, 2014-09-13 00:06:38]

lgtm % nits

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio.cc (right):

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:25: static void DummyMediaLog(const
std::string& s) {
nit: mp2t_stream_parser has a valid log_cb passed during Init(). During
RegisterPes(), pass it to this es parser during construction? Seems better to
include than defer as TODO. Or am I missing some complexity or other breakage
here? I've no strong feeling other than this looks like possible make-work.

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:131: if (remaining_size <
header.frame_size) {
|remaining_size| includes the size of the header. Is |header.frame_size|
expected to include the size of the frame *and* the header?

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:141: if (remaining_size >=
header.frame_size + 2 &&
nit: +2 or +1?

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:185: DVLOG(1) << "Sampling frequency:
" << header.sample_rate;
nit: also log the channel layout?

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio_unittest.cc (right):

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio_unittest.cc:52:
DCHECK_GT(stream_.size(), 0u);
nit: candidate for relocation into EsParserTestBase?

[damienv1, 2014-09-13 01:27:47]

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio.cc (right):

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:25: static void DummyMediaLog(const
std::string& s) {
On 2014/09/13 00:06:38, wolenetz wrote:
> nit: mp2t_stream_parser has a valid log_cb passed during Init(). During
> RegisterPes(), pass it to this es parser during construction? Seems better to
> include than defer as TODO. Or am I missing some complexity or other breakage
> here? I've no strong feeling other than this looks like possible make-work.

Done.

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:131: if (remaining_size <
header.frame_size) {
On 2014/09/13 00:06:38, wolenetz wrote:
> |remaining_size| includes the size of the header. Is |header.frame_size|
> expected to include the size of the frame *and* the header?

Yes. An MPEG-1 audio frame includes the header.
This is the size of the entire frame.

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:141: if (remaining_size >=
header.frame_size + 2 &&
On 2014/09/13 00:06:38, wolenetz wrote:
> nit: +2 or +1?

Right, that could be +1.

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio.cc:185: DVLOG(1) << "Sampling frequency:
" << header.sample_rate;
On 2014/09/13 00:06:38, wolenetz wrote:
> nit: also log the channel layout?

Done.

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_mpeg1audio_unittest.cc (right):

https://codereview.chromium.org/506943003/diff/80001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_mpeg1audio_unittest.cc:52:
DCHECK_GT(stream_.size(), 0u);
On 2014/09/13 00:06:38, wolenetz wrote:
> nit: candidate for relocation into EsParserTestBase?

Done.

[wolenetz, 2014-09-13 01:36:29]

lgtm again, thanks!

[damienv1, 2014-09-13 06:06:13]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-13 06:07:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/506943003/120001

[commit-bot: I haz the power, 2014-09-13 06:56:06]

Committed patchset #7 (id:120001) as 01cfaffd20934e8d07960d14e76ab2a4b17895db

[commit-bot: I haz the power, 2014-09-13 06:59:32]

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