Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773793002:
Allow mp3 audio codec for mpeg2ts containers in MSE (Closed)
DescriptionAllow mp3 audio codec for mpeg2ts containers in MSE
Some Chromecast apps use H.264 video + MP3 audio in mpeg2ts container.
BUG=704638
Review-Url: https://codereview.chromium.org/2773793002
Cr-Commit-Position: refs/heads/master@{#460170}
Committed: https://chromium.googlesource.com/chromium/src/+/9ad504653bc097753e522941f47d7ce761476706
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use mp4a.69,mp4a.6B codec ids instead of mp3 #
Total comments: 6
Patch Set 3 : CR feedback #
Messages
Total messages: 38 (24 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== Allow mp3 audio codec for mpeg2ts containers in MSE Some Chromecast apps use H.264 video + MP3 audio in mpeg2ts container. BUG=704638 ========== to ========== Allow mp3 audio codec for mpeg2ts containers in MSE Some Chromecast apps use H.264 video + MP3 audio in mpeg2ts container. BUG=704638 ==========
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, wolenetz@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by wolenetz@chromium.org
Please see comment w.r.t. standardization of the mp3 codec string itself in mp2ts: is "mp3" compliant with https://tools.ietf.org/html/rfc6381 ? https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:301: static const CodecInfo kMPEG2TS_MP3CodecInfo = {"mp3", CodecInfo::AUDIO, NULL, NOT LGTM until resolved: Is "mp3" standardized as a codec name for this mime type? For instance, in bug 661749, there is similar request for support for mp3 in mp4, with deliberation on codec string (none being mp3, candidates are among "mp4a.6B", "mp4a.69", and (less likely) "mp4a.40.some_x". Though mp2ts is Chromecast-specific, we probably want to still try to maintain in upstream at least some adherence to standard codec string names. See MSE bytestream registry for mp2ts's specifying user agents *should* use codec strings that conform to https://tools.ietf.org/html/rfc6381. https://codereview.chromium.org/2773793002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:115: const char kMPEG2TS_MP3Audio[] = "video/mp2t; codecs=\"avc1.4D4041,mp3\""; ditto
https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:336: {"video/mp2t", &BuildMP2TParser, kVideoMP2TCodecs}, also nit/aside query: why no audio/mp2t?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
servolk@chromium.org changed reviewers: + dougsteed@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:301: static const CodecInfo kMPEG2TS_MP3CodecInfo = {"mp3", CodecInfo::AUDIO, NULL, On 2017/03/23 20:17:39, wolenetz wrote: > NOT LGTM until resolved: > Is "mp3" standardized as a codec name for this mime type? For instance, in bug > 661749, there is similar request for support for mp3 in mp4, with deliberation > on codec string (none being mp3, candidates are among "mp4a.6B", "mp4a.69", and > (less likely) "mp4a.40.some_x". > > Though mp2ts is Chromecast-specific, we probably want to still try to maintain > in upstream at least some adherence to standard codec string names. > > See MSE bytestream registry for mp2ts's specifying user agents *should* use > codec strings that conform to https://tools.ietf.org/html/rfc6381. Ah, ok, as we've discussed offline, you are right - mp4a.6B and mp4a.69 seem to be more standard-compliant codec ids for mp3. I've update the CL to use those two codec ids instead of mp3. https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:336: {"video/mp2t", &BuildMP2TParser, kVideoMP2TCodecs}, On 2017/03/23 21:35:52, wolenetz wrote: > also nit/aside query: why no audio/mp2t? TBH, I don't know, that code was added by damienv@ long time ago. I guess none of the apps use the audio/mp2t type. Should we add it? https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:52: kStreamTypeMpeg2Audio = 0x4, +dougsteed@: Doug, I've added the stream_type=4 in here, this should work for the test streams you mentioned offline. I don't have the ITU H.222 spec, but many publicly-available sources seem to agree that 3 is ISO/IEC 11172-3 (MPEG-1 audio), while 4 is ISO/IEC 13818-3 (MPEG-2 audio), e.g.: https://en.wikipedia.org/wiki/Program-specific_information#Elementary_stream_... http://www.etherguidesystems.com/help/sdos/mpeg/semantics/mpeg-2/stream_type....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:52: kStreamTypeMpeg2Audio = 0x4, On 2017/03/23 23:24:59, servolk wrote: > +dougsteed@: Doug, I've added the stream_type=4 in here, this should work for > the test streams you mentioned offline. I don't have the ITU H.222 spec, but > many publicly-available sources seem to agree that 3 is ISO/IEC 11172-3 (MPEG-1 > audio), while 4 is ISO/IEC 13818-3 (MPEG-2 audio), e.g.: > https://en.wikipedia.org/wiki/Program-specific_information#Elementary_stream_... > http://www.etherguidesystems.com/help/sdos/mpeg/semantics/mpeg-2/stream_type.... So the formats are compatible? Note that the parser is called EsParserMpeg1Audio but now it is being asked to parse both.
On 2017/03/24 02:14:47, dougsteed wrote: > https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... > File media/formats/mp2t/mp2t_stream_parser.cc (right): > > https://codereview.chromium.org/2773793002/diff/20001/media/formats/mp2t/mp2t... > media/formats/mp2t/mp2t_stream_parser.cc:52: kStreamTypeMpeg2Audio = 0x4, > On 2017/03/23 23:24:59, servolk wrote: > > +dougsteed@: Doug, I've added the stream_type=4 in here, this should work for > > the test streams you mentioned offline. I don't have the ITU H.222 spec, but > > many publicly-available sources seem to agree that 3 is ISO/IEC 11172-3 > (MPEG-1 > > audio), while 4 is ISO/IEC 13818-3 (MPEG-2 audio), e.g.: > > > https://en.wikipedia.org/wiki/Program-specific_information#Elementary_stream_... > > > http://www.etherguidesystems.com/help/sdos/mpeg/semantics/mpeg-2/stream_type.... > > So the formats are compatible? Note that the parser is called EsParserMpeg1Audio > but now it is being asked to parse both. Yes, the formats are compatible. The main difference between the two is in supported sampling rates and bit rates. Here is a quote from https://en.wikipedia.org/wiki/MP3: MPEG-1 Audio (MPEG-1 Part 3), which included MPEG-1 Audio Layer I, II and III was approved as a committee draft of ISO/IEC standard in 1991,[10][11] finalised in 1992[12] and published in 1993 (ISO/IEC 11172-3:1993[5]). A backwards compatible MPEG-2 Audio (MPEG-2 Part 3) extension with lower sample and bit rates was published in 1995 (ISO/IEC 13818-3:1995) (see also http://www.datavoyage.com/mpgscript/mpeghdr.htm).
lgtm
lgtm % nits https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:336: {"video/mp2t", &BuildMP2TParser, kVideoMP2TCodecs}, On 2017/03/23 23:24:59, servolk wrote: > On 2017/03/23 21:35:52, wolenetz wrote: > > also nit/aside query: why no audio/mp2t? > > TBH, I don't know, that code was added by damienv@ long time ago. I guess none > of the apps use the audio/mp2t type. Should we add it? I was just curious. Since this is Chromecast specific, and apparently lack of audio/mp2t hasn't been an issue there (nor need fixing in this particular CL if it were an issue), probably ok not to add it. https://codereview.chromium.org/2773793002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:301: static const CodecInfo kMPEG2TS_MP3CodecInfo1 = { nit: please reference the mp4ra object registry site and ideally also the 13818-#/etc spec reference in comment for this and the Info2, since "69" and "6B" are not self-explanatory enough. https://codereview.chromium.org/2773793002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:2153: MockMediaSource source("bear-mp3-audio.ts", nit: is this test file really both .69 and .6B compliant, or do we need another test file?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/1/media/filters/stream_parser... media/filters/stream_parser_factory.cc:336: {"video/mp2t", &BuildMP2TParser, kVideoMP2TCodecs}, On 2017/03/24 17:34:25, wolenetz_ooo_Mar_27 wrote: > On 2017/03/23 23:24:59, servolk wrote: > > On 2017/03/23 21:35:52, wolenetz wrote: > > > also nit/aside query: why no audio/mp2t? > > > > TBH, I don't know, that code was added by damienv@ long time ago. I guess none > > of the apps use the audio/mp2t type. Should we add it? > > I was just curious. Since this is Chromecast specific, and apparently lack of > audio/mp2t hasn't been an issue there (nor need fixing in this particular CL if > it were an issue), probably ok not to add it. Acknowledged. https://codereview.chromium.org/2773793002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:301: static const CodecInfo kMPEG2TS_MP3CodecInfo1 = { On 2017/03/24 17:34:25, wolenetz_ooo_Mar_27 wrote: > nit: please reference the mp4ra object registry site and ideally also the > 13818-#/etc spec reference in comment for this and the Info2, since "69" and > "6B" are not self-explanatory enough. Done. https://codereview.chromium.org/2773793002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2773793002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:2153: MockMediaSource source("bear-mp3-audio.ts", On 2017/03/24 17:34:25, wolenetz_ooo_Mar_27 wrote: > nit: is this test file really both .69 and .6B compliant, or do we need another > test file? I've made a couple of new test files in the latest patchset, PTAL. IIUC the only difference between the two are in allowed sampling rates and bitrates. FFmpeg seems to make a choice solely based on sampling rate (see https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mpegtsenc...). So I've made two files - one with a sampling rate of 44100 and another one with 22050. This will allow us to test also the change in mp2t_stream_parser.cc
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm3 :)
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, dougsteed@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2773793002/#ps40001 (title: "CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490724900362490,
"parent_rev": "65976c977c5695514909ff1cebe30e2dd6c2c60c", "commit_rev":
"9ad504653bc097753e522941f47d7ce761476706"}
Message was sent while issue was closed.
Description was changed from ========== Allow mp3 audio codec for mpeg2ts containers in MSE Some Chromecast apps use H.264 video + MP3 audio in mpeg2ts container. BUG=704638 ========== to ========== Allow mp3 audio codec for mpeg2ts containers in MSE Some Chromecast apps use H.264 video + MP3 audio in mpeg2ts container. BUG=704638 Review-Url: https://codereview.chromium.org/2773793002 Cr-Commit-Position: refs/heads/master@{#460170} Committed: https://chromium.googlesource.com/chromium/src/+/9ad504653bc097753e522941f47d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9ad504653bc097753e522941f47d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
