Issue 23454006: Implement experimental MP3 support for Media Source API. - Code Review

Chromium Code Reviews

chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

(262)

My Issues | Starred Open | Closed | All

Issue 23454006: Implement experimental MP3 support for Media Source API. (Closed)

Created:
7 years, 3 months ago by acolwell GONE FROM CHROMIUM

Modified:
7 years, 3 months ago

Reviewers:
Ken Russell (switch to Gerrit), scherkus (not reviewing), jar (doing other things), akalin

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Implement experimental MP3 support for Media Source API. BUG=280550 TEST=PipelineIntegrationTest.MediaSource_MP3 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221471

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address CR comments. #

Patch Set 3 : Fix build busters #

Total comments: 42

Patch Set 4 : Address CR comments. #

Patch Set 5 : Added Icecast test. #

Total comments: 8

Patch Set 6 : Address CR comments. #

Patch Set 7 : Rebase #

Created: 7 years, 3 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+784 lines, -7 lines)			Patch
	M	content/browser/renderer_host/render_process_host_impl.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
	M	media/DEPS	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
	M	media/base/media_switches.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
	M	media/base/media_switches.cc	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
	M	media/base/run_all_unittests.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
	M	media/filters/pipeline_integration_test.cc	View	1 2 3 4	4 chunks	+44 lines, -5 lines	0 comments	Download
	M	media/filters/stream_parser_factory.h	View		1 chunk	+1 line, -1 line	0 comments	Download
	M	media/filters/stream_parser_factory.cc	View	1 2 3	8 chunks	+42 lines, -1 line	0 comments	Download
	M	media/media.gyp	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download
	A	media/mp3/mp3_stream_parser.h	View	1 2 3	1 chunk	+119 lines, -0 lines	0 comments	Download
	A	media/mp3/mp3_stream_parser.cc	View	1 2 3 4 5	1 chunk	+565 lines, -0 lines	0 comments	Download
	M	tools/metrics/histograms/histograms.xml	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

scherkus (not reviewing)

here's a quick first round -- I haven't done in detail review of mp3 code ...

7 years, 3 months ago (2013-08-30 00:06:11 UTC) #2

acolwell GONE FROM CHROMIUM

https://codereview.chromium.org/23454006/diff/1/media/filters/pipeline_integration_test.cc File media/filters/pipeline_integration_test.cc (right): https://codereview.chromium.org/23454006/diff/1/media/filters/pipeline_integration_test.cc#newcode637 media/filters/pipeline_integration_test.cc:637: switches::kEnableMP3StreamParser); On 2013/08/30 00:06:11, scherkus wrote: > this is ...

7 years, 3 months ago (2013-08-30 01:42:21 UTC) #3

scherkus (not reviewing)

https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h#newcode25 media/base/media_log.h:25: class MEDIA_EXPORT LogHelper { is this change needed? I ...

7 years, 3 months ago (2013-09-03 19:34:21 UTC) #4

acolwell GONE FROM CHROMIUM

https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h#newcode25 media/base/media_log.h:25: class MEDIA_EXPORT LogHelper { On 2013/09/03 19:34:22, scherkus wrote: ...

7 years, 3 months ago (2013-09-04 00:48:13 UTC) #5

https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h
File media/base/media_log.h (right):

https://codereview.chromium.org/23454006/diff/22001/media/base/media_log.h#ne...
media/base/media_log.h:25: class MEDIA_EXPORT LogHelper {
On 2013/09/03 19:34:22, scherkus wrote:
> is this change needed? I don't see any new references to LogHelper outside of
> the media target in this CL
Nope. This was only needed when I was doing some debugging in
PipelineIntegration test.

https://codereview.chromium.org/23454006/diff/22001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/23454006/diff/22001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:295: size_t quote2 =
mimetype_.find("\"", quote1 + 1);
On 2013/09/03 19:34:22, scherkus wrote:
> I know this is moving code around, but is it possible for quote1 and/or quote2
> to be std::string::npos?
Only if someone puts an invalid mimetype in mimetype constants above.

> 
> are these strings checked/sanitized elsewhere?
No. The assumption is that whoever created the mimetype constants above created
valid mimetypes.

I've made the code a little more robust and will fail hard if the assumptions
are violated.

https://codereview.chromium.org/23454006/diff/22001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:296: std::string codecStr =
mimetype_.substr(quote1 + 1, quote2 - quote1 - 1);
On 2013/09/03 19:34:22, scherkus wrote:
> s/codecStr/codec/ ? (should be unix_hacker style)

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
File media/mp3/mp3_stream_parser.cc (right):

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:19: static const uint32 kICYStartCode =
0x49435920; // 'ICY '
On 2013/09/03 19:34:22, scherkus wrote:
> does this permit appending of icecast streams?
Yes.

> 
> should we add a test for that?
Yeah. I'll add a version of sfx.mp3 with an icecast header prepended to it.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:20: static const int kMaxIcecastHeaderSize =
4096;
On 2013/09/03 19:34:22, scherkus wrote:
> is this arbitrary? if so, I'd mention it is

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:28: // Map that determined which bitrate_index &
channel_mode combinations
On 2013/09/03 19:34:22, scherkus wrote:
> s/determined/determines/?

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:188: int MP3StreamParser::ParseFrameHeader(const
uint8* data, int size,
On 2013/09/03 19:34:22, scherkus wrote:
> there's a lot of magic constants in here that might be worth beefing up w/
docs
> and/or URLs to the spec and/or units

Done.

> 
> for example, that 1152 samples is a constant as defined in the spec for
version
> 3 and how the value 144 relates to that number
> 
> also I think that "144" needs to be "72" when version != 3 :)

I've updated the code w/ help from the Internets and FFmpeg code. The necessary
change was a little more involved, but you were right that the sample count
wasn't right. I think the results are a little more readable now.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:286: (channel_mode == 3) ? CHANNEL_LAYOUT_MONO :
CHANNEL_LAYOUT_STEREO;
On 2013/09/03 19:34:22, scherkus wrote:
> for this one it might be nice to call out that stereo (0), joint stereo (1),
and
> dual channel (2) all get collapsed into stereo output since that's information
> for a decoder, etc...

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:317: (config_.samples_per_second() != sample_rate
||
On 2013/09/03 19:34:22, scherkus wrote:
> I thought MP3s maintained the same sample rate / channel layout
I believe this is only by convention. I believe that compliant decoders are
supposed to handle changes in these fields, but early ones probably didn't so
people avoided changing them in a single file.

> 
> does this mean we can append mp3 frame data with changing audio configs?
Yes. This makes these changes look like what would happen if vorbis w/ different
sample rates * channel layouts were appended. That way once we actually support
sample rate changes, it should... "just work".... :)

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:353: // possible before calling new_buffers_cb_.
On 2013/09/03 19:34:22, scherkus wrote:
> |new_buffers_cb_|

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:367: DVLOG(1) << "ParseIcecastHeader(" << size <<
")";
On 2013/09/03 19:34:22, scherkus wrote:
> __FUNCTION__

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:372: if (memcmp("ICY ", data, 4))
On 2013/09/03 19:34:22, scherkus wrote:
> after checking for ICY I believe you can use HttpUtil::LocateEndOfHeaders() to
> grab the int offset
> 
> doing so would require putting an explicit dependency on media in media.gyp
and
> media/DEPS (AFAIK we already depend on net implicitly since media depends on
> ui.gyp:ui, which depends on net.gyp:net)

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:375: const uint8* end = data + size;
On 2013/09/03 19:34:22, scherkus wrote:
> would it be preferable to cast start/end to const char* and use str functions
> instead of mem?
The str functions would treat \0 in a special way. We don't want that.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:376: for (const uint8* start = data; start < end;
++start) {
On 2013/09/03 19:34:22, scherkus wrote:
> s/start/pos/?
> 
> my initial scan through this code made me think start wasn't going to be
> modified

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:383: // TODO(acolwell): Add code to actually
parse header lines.
On 2013/09/03 19:34:22, scherkus wrote:
> OOC what sort of information can we get out of the headers?
Title, author, copyright, bitrate... and whether the inband metadata feature is
turned on. In a followup CL I may try to add support for the inband metadata,
but it will likely be a PitA so parsing the headers would at least allow us to
reject such streams if we happened to encounter them.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:398: MEDIA_LOG(log_cb_) << "Icecast header is too
large. ";
On 2013/09/03 19:34:22, scherkus wrote:
> remove extra trailing space in " "

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:413: return (!memcmp(data, "TAG+", 4)) ?
kID3v1ExtendedSize : kID3v1Size;
On 2013/09/03 19:34:22, scherkus wrote:
> nit: remove the extra () in first clause?

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.cc:483: // Try to find 3 valid frames in a row.
On 2013/09/03 19:34:22, scherkus wrote:
> OOC why 3?
Updated the comment. Checking for only 2 headers in a row can lead to false
positives. 3 decreases this probability.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
File media/mp3/mp3_stream_parser.h (right):

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.h:23: namespace mp3 {
On 2013/09/03 19:34:22, scherkus wrote:
> FYI / personal pet-peeve but this is a bad example of nested namespace usage
> since the class you're namespacing is also prefixed with the namespace name
...
> but I'd go w/ the consistency here and stick with things as is since we use
> nested namespaces for other StreamParsers
> 
>
https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/RVps606IB...

I'll remove the namespace. The WebM parser doesn't live in its own namespace.
Only the MP4 parser does. We are also only talking about 1 class here so a
namespace seems like overkill anyways.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.h:30: virtual void Init(const InitCB& init_cb, const
NewConfigCB& config_cb,
On 2013/09/03 19:34:22, scherkus wrote:
> // StreamParser implementation.

Done.

https://codereview.chromium.org/23454006/diff/22001/media/mp3/mp3_stream_pars...
media/mp3/mp3_stream_parser.h:76: int* sample_rate,
On 2013/09/03 19:34:22, scherkus wrote:
> document these last 3 args may be NULL

Done.

scherkus (not reviewing)

few nits but lgtm my memory is foggy, but you may need someone from net/ ...

7 years, 3 months ago (2013-09-04 01:25:37 UTC) #6

acolwell GONE FROM CHROMIUM

https://codereview.chromium.org/23454006/diff/34001/media/filters/pipeline_integration_test.cc File media/filters/pipeline_integration_test.cc (right): https://codereview.chromium.org/23454006/diff/34001/media/filters/pipeline_integration_test.cc#newcode289 media/filters/pipeline_integration_test.cc:289: // This code assumes that |mimetype_| is one of ...

7 years, 3 months ago (2013-09-04 01:36:01 UTC) #7

acolwell GONE FROM CHROMIUM

akalin: media/DEPS owners review. jar: tools/metrics/histograms/histograms.xml kbr: content/browser/renderer_host OWNERS

7 years, 3 months ago (2013-09-04 15:55:41 UTC) #8

Ken Russell (switch to Gerrit)

I'm not officially an OWNER of that file, but the change is trivial, so content/browser/renderer_host ...

7 years, 3 months ago (2013-09-04 16:56:16 UTC) #10

acolwell GONE FROM CHROMIUM

On 2013/09/04 16:56:16, Ken Russell wrote: > I'm not officially an OWNER of that file, ...

7 years, 3 months ago (2013-09-04 17:56:42 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/23454006/46001

7 years, 3 months ago (2013-09-05 15:46:19 UTC) #13

Message was sent while issue was closed.

Change committed as 221471

Expand Messages | Collapse Messages

Powered by Google App Engine

This is Rietveld 408576698