Add FLAC audio codec support

Add FLAC audio codec support

BUG=93887

[servolk, 2016-09-23 22:14:03]

Description was changed from

==========
Add FLAC audio codec support

BUG=93887
==========

to

==========
Add FLAC audio codec support

BUG=93887
==========

[servolk, 2016-09-23 22:14:04]

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

[servolk, 2016-09-23 22:17:16]

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

https://codereview.chromium.org/2362193003/diff/1/media/base/mime_util_intern...
media/base/mime_util_internal.cc:418:
AddContainerWithCodecs("application/x-flac", flac_audio_codecs, false);
Firefox supports all 3 mime types
(https://reviewboard.mozilla.org/r/72408/diff/1/?file=2074198#file2074198), so
I've also added all 3 for now.

[servolk, 2016-09-23 22:19:52]

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

[servolk, 2016-09-23 22:20:44]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 22:21:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:00:51]

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

[commit-bot: I haz the power, 2016-09-23 23:00:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-09-24 00:15:30]

I think we have a few flac tests in other places (at least a browser test) that
can come out from behind the OS_CHROMEOS flag now. This will have to wait for
the ffmpeg update obviously.

[servolk, 2016-09-24 00:58:43]

On 2016/09/24 00:15:30, DaleCurtis wrote:
> I think we have a few flac tests in other places (at least a browser test)
that
> can come out from behind the OS_CHROMEOS flag now. This will have to wait for
> the ffmpeg update obviously.

Unblocked tests in audio_decoder_unittest.cc and demuxer_perftest.cc in PS #2.

[DaleCurtis, 2016-09-26 16:44:14]

Add
https://cs.chromium.org/chromium/src/content/browser/media/media_browsertest.cc
too?

[servolk, 2016-09-26 16:52:47]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 16:53:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-26 16:53:26]

On 2016/09/26 16:44:14, DaleCurtis wrote:
> Add
>
https://cs.chromium.org/chromium/src/content/browser/media/media_browsertest.cc
> too?

Done.

[DaleCurtis, 2016-09-26 16:57:16]

lg2m, will stamp once we can actually roll deps.

[commit-bot: I haz the power, 2016-09-26 17:42:37]

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

[commit-bot: I haz the power, 2016-09-26 17:42:38]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wolenetz, 2016-10-24 22:20:14]

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,
aside: how was this position in the enum determined?

[servolk, 2016-10-24 22:26:05]

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/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.

[ddorwin, 2016-11-16 17:44:46]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2016-11-16 17:44:47]

Please clarify in the first line of the commit message that this is the FLAC
codec *in the FLAC [container] format*. Specifically, this does not add FLAC in
Ogg or MP4.

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:417: AddContainerWithCodecs("audio/x-flac",
flac_audio_codecs, false);
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).

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:573: case FLAC:
Is this actually supported by MediaPlayer? MediaCodec?

Please try to order consistently with the definition.

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/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.

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:
Ditto on expectations and order.

[ddorwin, 2016-11-17 17:46:31]

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},
We don't need this since we aren't currently supporting "flac" as a codec
independent of the FLAC format. See below.

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:370: CodecSet flac_audio_codecs;
We don't need this. Instead, update GetDefaultCodecLowerCase.

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);
s/flac_audio_codecs/implicit_codec/

We don't want 'audio/flac; codecs=flac'. Instead, we need to treat this like
'audio/mp3'.

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/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...

https://codereview.chromium.org/2362193003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:778: case FLAC:
FLAC should be added to all the tests in
content/browser/media/media_canplaytype_browsertest.cc.
See "audio/mp3" for examples of codec-less types. But also include tests for
"flac" in the codecs parameter for all other container types.

Also, there was a layout test but it was removed. I don't know how much we care
about that if we have have browser tests.
https://codereview.chromium.org/2028463002/diff/40001/third_party/WebKit/Layo...

[DaleCurtis, 2016-11-17 23:41:58]

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.