Skip audio frames with unknown timestamp in MPEG2TS

Skip audio frames with unknown timestamp in MPEG2TS

This is another issue with HLS streams produced by VLC. Even though it's recommended that each PES packet in MPEG2TS contain a timestamp, we have seen that in the real-world content streams that's not always the case. In order to be able to play such streams, instead of failing stream parsing when we don't have timing info at the beginning of playback, we will just skip some frames until we eventually  get a frame with timing info and from that point we should be able to proceed playing normally. Previous change by Damien fixed this issue for video streams. This change fixes it for AAC ADTS and MPEG1 audio streams in mpeg2 ts containers.

BUG=420227
Committed: https://crrev.com/bf74c81c39175cfe5cca4f559f10999bca19af54
Cr-Commit-Position: refs/heads/master@{#298982}

[servolk, 2014-10-09 18:13:25]

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

[damienv1, 2014-10-09 18:29:09]

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

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:143: DVLOG(1) << "Skipped audio frame with
unknown timestamp";
nit: would prefer the present. "Skip audio frame...

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:144: SkipAdtsFrame(adts_frame);
Could you update the unit test as well ?

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:145: continue;
To be consistent, you should do the same for the MPEG1 audio ES parser.
At some point, we might want to have an EsAdapterAudio.

[servolk, 2014-10-09 18:53:04]

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

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:143: DVLOG(1) << "Skipped audio frame with
unknown timestamp";
On 2014/10/09 18:29:09, damienv1 wrote:
> nit: would prefer the present. "Skip audio frame...

Done.

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:144: SkipAdtsFrame(adts_frame);
On 2014/10/09 18:29:09, damienv1 wrote:
> Could you update the unit test as well ?

Done.

https://codereview.chromium.org/646673002/diff/1/media/formats/mp2t/es_parser...
media/formats/mp2t/es_parser_adts.cc:145: continue;
On 2014/10/09 18:29:09, damienv1 wrote:
> To be consistent, you should do the same for the MPEG1 audio ES parser.
> At some point, we might want to have an EsAdapterAudio.

Done.

[wolenetz, 2014-10-09 19:12:46]

Also: please clarify in the CL description this is specific to MP2TS, and not
the ADTSStreamParser or MPEG1AudioStreamParser, including why (I assume from the
scope of this CL that the lack of timing information is specific to ES timing
info, and neither of the two stream parsers, above, operate on ES, right?)

Otherwise, looking pretty good % tiny nit:

https://codereview.chromium.org/646673002/diff/70001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/646673002/diff/70001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:143: DVLOG(1) << "Skip audio frame with
unknown timestamp";
nit: s/Skip/Skipping/ :)
(and ditto in the mpeg1audio parser)

[servolk, 2014-10-09 20:14:12]

Updated description to better describe the issue and be more clear that this is
only for mpeg2ts containers.

https://codereview.chromium.org/646673002/diff/70001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/646673002/diff/70001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:143: DVLOG(1) << "Skip audio frame with
unknown timestamp";
On 2014/10/09 19:12:46, wolenetz wrote:
> nit: s/Skip/Skipping/ :)
> (and ditto in the mpeg1audio parser)

Done.

[damienv1, 2014-10-09 20:22:15]

lgtm

[servolk, 2014-10-09 20:22:45]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:23:50]

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

[commit-bot: I haz the power, 2014-10-09 20:32:35]

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

[commit-bot: I haz the power, 2014-10-09 20:32:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wolenetz, 2014-10-09 21:09:04]

sorry. lgtm! ship it :)

[servolk, 2014-10-09 21:11:59]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2014-10-09 21:12:42]

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

[servolk, 2014-10-09 21:15:10]

On 2014/10/09 21:09:04, wolenetz wrote:
> sorry. lgtm! ship it :)

Thanks!
Sorry, I though Damien was one of the owners, so didn't realize I needed to wait
for lgtm from you.
Re-trying now.

[wolenetz, 2014-10-09 21:16:21]

On 2014/10/09 21:15:10, servolk wrote:
> On 2014/10/09 21:09:04, wolenetz wrote:
> > sorry. lgtm! ship it :)
> 
> Thanks!
> Sorry, I though Damien was one of the owners, so didn't realize I needed to
wait
> for lgtm from you.
> Re-trying now.

no worries. I only had nits so should have l-g-t-m'ed it on my first pass.

[commit-bot: I haz the power, 2014-10-09 21:31:40]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2014-10-09 21:32:21]

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