Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2362193003: Add FLAC audio codec support (Closed)

Created:
2 years, 7 months ago by servolk
Modified:
2 years, 6 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, mlamouri (slow - plz ping), posciak+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add FLAC audio codec support BUG=93887

Patch Set 1 #

Total comments: 1

Patch Set 2 : Unblock existing test that use FLAC #

Patch Set 3 : Enabled the test in media_browsertest.cc on Chromium #

Total comments: 18
Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -11 lines) Patch
M content/browser/media/media_browsertest.cc View 1 2 1 chunk +3 lines, -4 lines 0 comments Download
M media/base/demuxer_perftest.cc View 1 1 chunk +1 line, -3 lines 0 comments Download
M media/base/mime_util_internal.h View 1 chunk +1 line, -0 lines 4 comments Download
M media/base/mime_util_internal.cc View 5 chunks +10 lines, -0 lines 12 comments Download
M media/base/mime_util_unittest.cc View 3 chunks +3 lines, -0 lines 2 comments Download
M media/filters/audio_decoder_unittest.cc View 1 2 chunks +0 lines, -4 lines 0 comments Download
M media/filters/ffmpeg_demuxer_unittest.cc View 1 chunk +20 lines, -0 lines 0 comments Download
M media/test/pipeline_integration_test.cc View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (12 generated)
servolk
https://codereview.chromium.org/2362193003/diff/1/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2362193003/diff/1/media/base/mime_util_internal.cc#newcode418 media/base/mime_util_internal.cc:418: AddContainerWithCodecs("application/x-flac", flac_audio_codecs, false); Firefox supports all 3 mime types ...
2 years, 7 months ago (2016-09-23 22:17:16 UTC) #3
servolk
2 years, 7 months ago (2016-09-23 22:19:59 UTC) #5
DaleCurtis
I think we have a few flac tests in other places (at least a browser ...
2 years, 7 months ago (2016-09-24 00:15:30 UTC) #10
servolk
On 2016/09/24 00:15:30, DaleCurtis wrote: > I think we have a few flac tests in ...
2 years, 7 months ago (2016-09-24 00:58:43 UTC) #11
DaleCurtis
Add https://cs.chromium.org/chromium/src/content/browser/media/media_browsertest.cc too?
2 years, 7 months ago (2016-09-26 16:44:14 UTC) #12
servolk
On 2016/09/26 16:44:14, DaleCurtis wrote: > Add > https://cs.chromium.org/chromium/src/content/browser/media/media_browsertest.cc > too? Done.
2 years, 7 months ago (2016-09-26 16:53:26 UTC) #15
DaleCurtis
lg2m, will stamp once we can actually roll deps.
2 years, 7 months ago (2016-09-26 16:57:16 UTC) #16
wolenetz
https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.h File media/base/mime_util_internal.h (right): https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.h#newcode37 media/base/mime_util_internal.h:37: FLAC, aside: how was this position in the enum ...
2 years, 6 months ago (2016-10-24 22:20:14 UTC) #19
servolk
https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.h File media/base/mime_util_internal.h (right): https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.h#newcode37 media/base/mime_util_internal.h:37: FLAC, On 2016/10/24 22:20:14, wolenetz wrote: > aside: how ...
2 years, 6 months ago (2016-10-24 22:26:05 UTC) #20
ddorwin
Please clarify in the first line of the commit message that this is the FLAC ...
2 years, 6 months ago (2016-11-16 17:44:47 UTC) #22
ddorwin
https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_internal.cc#newcode36 media/base/mime_util_internal.cc:36: {"flac", MimeUtil::FLAC}, We don't need this since we aren't ...
2 years, 6 months ago (2016-11-17 17:46:31 UTC) #23
DaleCurtis
2 years, 6 months ago (2016-11-17 23:41:58 UTC) #24
Since servolk is out@ updated and addressed comments here
https://codereview.chromium.org/2515553002

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:36: {"flac", MimeUtil::FLAC},
On 2016/11/17 at 17:46:31, ddorwin wrote:
> We don't need this since we aren't currently supporting "flac" as a codec
independent of the FLAC format. See below.

Removed.

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:370: CodecSet flac_audio_codecs;
On 2016/11/17 at 17:46:31, ddorwin wrote:
> We don't need this. Instead, update GetDefaultCodecLowerCase.

Removed.

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:416: AddContainerWithCodecs("audio/flac",
flac_audio_codecs, false);
On 2016/11/17 at 17:46:31, ddorwin wrote:
> s/flac_audio_codecs/implicit_codec/
> 
> We don't want 'audio/flac; codecs=flac'. Instead, we need to treat this like
'audio/mp3'.

Done.

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:417: AddContainerWithCodecs("audio/x-flac",
flac_audio_codecs, false);
On 2016/11/17 at 17:46:31, ddorwin wrote:
> On 2016/11/16 17:44:47, ddorwin wrote:
> > Do we need to support these other variants? Are there many sites using
these? x-
> > variants? Unless there are a lot of legacy uses (seems unlikely given the
lack
> > of browser support), we should try to stick to one. Maybe add UMAs to
determine
> > if we need others.
> > 
> > Firefox claims to only support the first two and prefer the first:
> >
https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats#FLAC
> > However, the code actually supports all three:
> >
https://dxr.mozilla.org/mozilla-central/source/dom/media/flac/FlacDecoder.cpp
> > 
> > Maybe we should reach out to find out why and coordinate. Let me know if you
> > need a contact.
> > 
> > For completeness:
> > * https://xiph.org/flac/ does not appear to mention a type.
> > * http://www.digitalpreservation.gov/formats/fdd/fdd000198.shtml specifies
> > "audio/flac"
> > * "flac" does not appear in
> > http://www.iana.org/assignments/media-types/media-types.xhtml
> > * https://en.wikipedia.org/wiki/FLAC#cite_ref-1 says, "Registration being
sought
> > as audio/flac" but provides no reference. That text was added in 2007
> > (https://en.wikipedia.org/w/index.php?title=FLAC&diff=prev&oldid=164696923).
> 
> FWIW, this would also be inconsistent with net/:
https://cs.chromium.org/chromium/src/net/base/mime_util.cc?q=audio/flac&sq=pa...

Made consistent with net, i.e. just audio/flac

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:573: case FLAC:
On 2016/11/16 at 17:44:47, ddorwin wrote:
> Is this actually supported by MediaPlayer? MediaCodec?
> 
> Please try to order consistently with the definition.

Yes, it's supported by Android platform always.

https://developer.android.com/guide/appendix/media-formats.html

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
File media/base/mime_util_internal.h (right):

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.h:37: FLAC,
On 2016/11/16 at 17:44:47, ddorwin wrote:
> On 2016/10/24 22:26:05, servolk wrote:
> > On 2016/10/24 22:20:14, wolenetz wrote:
> > > aside: how was this position in the enum determined?
> > 
> > Oh, this CL is about a month old now, I don't remember TBH. Perhaps I
thought
> > that Vorbis was the last video codec in this list and FLAC would be the
first
> > audio codec. But looking closer at the list now, the ordering here seems to
be
> > completely random.
> 
> It is audio then video. This should probably go after OPUS.

Doesn't matter, but done.

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_un...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:263: case MimeUtil::FLAC:
On 2016/11/16 at 17:44:47, ddorwin wrote:
> Ditto on expectations and order.

Acknowledged.

Powered by Google App Engine
This is Rietveld 408576698