1195723 - FLAC support / Create FLAC MediaDataDemuxer

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, enhancement, P5)

Description

[Diogo Pereira]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36




Expected results:

It would be great if FLAC was supported in audio elements.

This is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=586568 but cajbir suggested opening a new bug just for FLAC.

It looks like Chromium may land this soon, see https://code.google.com/p/chromium/issues/detail?id=93887

Comment 1

[Gingerbread Man]

Technically a duplicate of bug 514365, but if another browser will add FLAC support, the issue may be worth revisiting.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The intent of the js decoders is to cover this kind of thing.

Comment 3

[Matthew Gregan [:kinetik]]

FWIW, Tidal is serving FLAC to Chrome using a PNaCl FLAC decoder for "HiFi" streams (some details about why they do this rather than use a pure JS decoder in https://github.com/audiocogs/flac.js/issues/8).  Firefox isn't offered this quality level, only "Normal" and "High", which are using lossy codecs via Flash plugin (not sure why they don't use native playback, presumably it's just MP3 or AAC).

Comment 4

[Mardeg]

(In reply to Gingerbread Man from comment #1)
> Technically a duplicate of bug 514365, but if another browser will add FLAC
> support, the issue may be worth revisiting.

Duping 514365 forward to this one.

Comment 6

[mattia.b89]

I confirm FLAC doesn't work

I comment in order to share with you an easy test case:
http://hpr.dogphilosophy.net/test/

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P1. Primitive flac frame parser.

Currently only handle metadata decoding.

Review commit: https://reviewboard.mozilla.org/r/71018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71018/

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

Make it just like MP4, WebM and all the others new demuxers.
Additionally, make the ogg related preferences part of MediaPrefs.

Review commit: https://reviewboard.mozilla.org/r/71020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71020/

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

This feature is intended to debug the flac parser only and is behind a hidden pref.
There's lots of redundant code in OggCodecState, there's need for a serious cleanup there.

Review commit: https://reviewboard.mozilla.org/r/71022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71022/

Comment 10

[Jean-Yves Avenard [:jya]]

the file in testing/web-platform/tests/media/A4.ogv is a theora/flac file ; though not particularly useful.

I used the flac sample file there http://www.eclassical.com/pages/24-bit-faq.html and converted them to ogg with:
ffmpeg -i BIS1447-002-flac_16.flac -acodec copy BIS1447-002-flac_16.ogg

tested with flac 16 bits and 24 bits, 44.1kHz and 88.2kHz.

Comment 11

[u480271]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

https://reviewboard.mozilla.org/r/71022/#review68634

::: dom/media/ogg/OggCodecState.h:623
(Diff revision 1)
> +  nsresult PageIn(ogg_page* aPage) override;
> +
> +  // Return a hash table with tag metadata.
> +  MetadataTags* GetTags() override;
> +
> +  AudioInfo& GetInfo();

can this be const AudioInfo& GetInfo()?

::: dom/media/ogg/OggCodecState.cpp:1245
(Diff revision 1)
> +bool
> +FlacState::DecodeHeader(ogg_packet* aPacket)
> +{
> +  nsAutoRef<ogg_packet> autoRelease(aPacket);
> +
> +  if (!mParser.DecodeHeaderBlock(aPacket->packet, aPacket->bytes)) {

From my understanding of https://xiph.org/flac/ogg_mapping.html and looking at example flac-in-ogg, when this is called, aPacket begins with:

  `0x7f + FLAC + MAJOR + MINOR + HEADER_COUNT + fLaC + METADATABLOCK + ...`

Why not strip off the 9 bytes header here and pass:

  `flac + METADATA_BLOCK + ...`
  
into mParser.DecoderHeaderBlock()?

::: dom/media/ogg/OggCodecState.cpp:1260
(Diff revision 1)
> +FlacState::Time(int64_t granulepos)
> +{
> +  if (!mParser.mInfo.IsValid()) {
> +    return -1;
> +  }
> +  CheckedInt64 t =

nit: Too long for one line?

::: dom/media/ogg/OggCodecState.cpp:1311
(Diff revision 1)
> +  }
> +  return NS_OK;
> +}
> +
> +// Return a hash table with tag metadata.
> +MetadataTags*

const MetadataTags*? or is it intended that caller might modify the hash table? (I see it's an override from base class)

::: dom/media/ogg/OggCodecState.cpp:1327
(Diff revision 1)
> +
> +bool
> +FlacState::ReconstructFlacGranulepos(void)
> +{
> +  NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
> +  ogg_packet* last = mUnstamped[mUnstamped.Length()-1];

`ogg_packet* last = mUnstamped.LastElement();`

::: dom/media/ogg/OggDemuxer.h:267
(Diff revision 1)
>    VorbisState* mVorbisState;
>  
>    // Decode state of the Opus bitstream we're decoding, if we have one.
>    OpusState* mOpusState;
>  
>    // Get the bitstream decode state for the given track type

Should this comment go with GetTrackCodecState?

Comment 12

[u480271]

Comment on attachment 8780309 [details]
Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

https://reviewboard.mozilla.org/r/71020/#review68640

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

https://reviewboard.mozilla.org/r/71022/#review68634

> From my understanding of https://xiph.org/flac/ogg_mapping.html and looking at example flac-in-ogg, when this is called, aPacket begins with:
> 
>   `0x7f + FLAC + MAJOR + MINOR + HEADER_COUNT + fLaC + METADATABLOCK + ...`
> 
> Why not strip off the 9 bytes header here and pass:
> 
>   `flac + METADATA_BLOCK + ...`
>   
> into mParser.DecoderHeaderBlock()?

but I do want to verify that those 9 bytes are correct!
if I were to simply strip them, how would I verify the validity of the content ?

> const MetadataTags*? or is it intended that caller might modify the hash table? (I see it's an override from base class)

i'm extending an existing code, i did not write the API....

> `ogg_packet* last = mUnstamped.LastElement();`

this is the exact same code as found in all other CodecState implementation.

> Should this comment go with GetTrackCodecState?

oops

Comment 14

[u480271]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

https://reviewboard.mozilla.org/r/71018/#review68592

With bug fixed.

::: dom/media/FlacFrameParser.h:20
(Diff revision 1)
> +namespace mozilla
> +{
> +
> +class OpusParser;
> +
> +class FlacFrameParser

Maybe you could add a comment that this parses FLAC contained in Ogg and reference the spec https://xiph.org/flac/ogg_mapping.html?

::: dom/media/FlacFrameParser.h:40
(Diff revision 1)
> +  AudioInfo mInfo;
> +
> +private:
> +  bool ReconstructFlacGranulepos(void);
> +  Maybe<uint32_t> mNumHeaders;
> +  uint32_t mMinBlockSize;

These *could* be uint16_t.

::: dom/media/FlacFrameParser.cpp:71
(Diff revision 1)
> +  , mPacketCount(0)
> +{
> +}
> +
> +bool
> +FlacFrameParser::DecodeHeaderBlock(const uint8_t* aPacket, size_t aLength)

Maybe DecodeStreamHeader is a better name?

::: dom/media/FlacFrameParser.cpp:81
(Diff revision 1)
> +  }
> +  AutoByteReader br(aPacket, aLength);
> +
> +  mPacketCount++;
> +
> +  uint32_t blockType = br.ReadU8() & 0x7f;

I think this is a bug. r-

::: dom/media/FlacFrameParser.cpp:83
(Diff revision 1)
> +
> +  mPacketCount++;
> +
> +  uint32_t blockType = br.ReadU8() & 0x7f;
> +
> +  if (blockType == OGG_FLAC_METADATA_TYPE_STREAMINFO) {

if your current flac-in-ogg header is passed to this function, how is it valid to access blockType before stripping off `0x7F + FLAC`?

I think this code should check for `0x&F + FLAC` or `fLaC` and assert it.

Comment 15

[u480271]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

https://reviewboard.mozilla.org/r/71022/#review68654

After discussion on irc, that issues as suggestions.

Comment 16

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> Comment on attachment 8780310 [details]
> Bug 1195723: [ogg/flac] P3. Add flac support in ogg.
> 
> https://reviewboard.mozilla.org/r/71022/#review68634
> 
> > From my understanding of https://xiph.org/flac/ogg_mapping.html and looking at example flac-in-ogg, when this is called, aPacket begins with:
> > 
> >   `0x7f + FLAC + MAJOR + MINOR + HEADER_COUNT + fLaC + METADATABLOCK + ...`
> > 
> > Why not strip off the 9 bytes header here and pass:
> > 
> >   `flac + METADATA_BLOCK + ...`
> >   
> > into mParser.DecoderHeaderBlock()?
> 
> but I do want to verify that those 9 bytes are correct!
> if I were to simply strip them, how would I verify the validity of the
> content ?

My fault. My unspoken assumption was that you're check the 9 bytes, and then 'strip' them from the packet before handing them on to DecoderHeaderBlock().

I'll be explicit next time.

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

https://reviewboard.mozilla.org/r/71018/#review68592

> I think this is a bug. r-

why that? the first bit is a flag to indicate if it's the last metadata or not, and then you have the type

> if your current flac-in-ogg header is passed to this function, how is it valid to access blockType before stripping off `0x7F + FLAC`?
> 
> I think this code should check for `0x&F + FLAC` or `fLaC` and assert it.

That's what the first code block above does.

if (blockType == OGG_FLAC_METADATA_TYPE_STREAMINFO) {
 ... check that the ogg header is valid, and advance the pointer so that it points straight to the start of the STREAMINFO.
 
So we check that it starts by FLAC, that this ogg prefix is correct and will error if not.
it will also check that it's the first block seen and that it must contain a STREAMINFO.

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Gingerbread Man from comment #1)
> Technically a duplicate of bug 514365, but if another browser will add FLAC
> support, the issue may be worth revisiting.

this bug is primarily about adding support for flac, the raw container... the bug you pointed it so for flac in ogg.

Comment 19

[u480271]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

https://reviewboard.mozilla.org/r/71018/#review68592

> That's what the first code block above does.
> 
> if (blockType == OGG_FLAC_METADATA_TYPE_STREAMINFO) {
>  ... check that the ogg header is valid, and advance the pointer so that it points straight to the start of the STREAMINFO.
>  
> So we check that it starts by FLAC, that this ogg prefix is correct and will error if not.
> it will also check that it's the first block seen and that it must contain a STREAMINFO.

Sorry, I didn't refer back to the definition of OGG_FLAC_METADATA_TYPE_STREAMINFO. I confused it with METADATA_BLOCK_HEADER BLOCK_TYPE STREAMINFO from the Flac spec.

Comment 20

[u480271]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

https://reviewboard.mozilla.org/r/71018/#review68592

> why that? the first bit is a flag to indicate if it's the last metadata or not, and then you have the type

Because blockType sounds like https://xiph.org/flac/format.html#metadata_block_header

Comment 21

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [adts] P4. Add extra mimetype for adts/aac.

When the content is served by Apache, a file with an .aac extension always see its Content-Type set to x-aacp.

Review commit: https://reviewboard.mozilla.org/r/72400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72400/

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: P5. Add BitReader class.

Extracted from the H264 codec and using the stagefright one.

Review commit: https://reviewboard.mozilla.org/r/72402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72402/

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P6. Add support for raw flac metadata decoding.

We also move flac related files to their own flac folder.

Review commit: https://reviewboard.mozilla.org/r/72404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72404/

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P7. Add flac demuxer.

Review commit: https://reviewboard.mozilla.org/r/72406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72406/

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P8. Add flac MediaDecoder.

Review commit: https://reviewboard.mozilla.org/r/72408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72408/

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

Review commit: https://reviewboard.mozilla.org/r/72410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72410/

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P10. Add media sniffer for flac file.

This only supports flac with a STREAMINFO header.

Review commit: https://reviewboard.mozilla.org/r/72412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72412/

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

Considering its popular use, the decision to support this format was agreed upon.

Review commit: https://reviewboard.mozilla.org/r/72414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72414/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71018/diff/1-2/

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780309 [details]
Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71020/diff/1-2/

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71022/diff/1-2/

Comment 32

[u480271]

Comment on attachment 8782196 [details]
Bug 1195723: [adts] P4. Add extra mimetype for adts/aac.

https://reviewboard.mozilla.org/r/72400/#review70054

Comment 33

[u480271]

Comment on attachment 8782197 [details]
Bug 1195723: P5. Add BitReader class.

https://reviewboard.mozilla.org/r/72402/#review70062

Comment 34

[u480271]

Comment on attachment 8782198 [details]
Bug 1195723: [flac] P6. Add support for raw flac metadata decoding.

https://reviewboard.mozilla.org/r/72404/#review70064

Comment 35

[u480271]

Comment on attachment 8782199 [details]
Bug 1195723: [flac] P7. Add flac demuxer.

https://reviewboard.mozilla.org/r/72406/#review70070

::: dom/media/flac/FlacDemuxer.cpp:360
(Diff revision 1)
> +  void SetRate(uint32_t aRate) { mHeader.mInfo.mRate = aRate; };
> +
> +  void SetBitDepth(uint32_t aBitDepth) { mHeader.mInfo.mBitDepth = aBitDepth; }
> +
> +  void SetInvalid() { mHeader.mValid = false; }
> +  

Stray white space.

Comment 36

[u480271]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

https://reviewboard.mozilla.org/r/72408/#review70072

::: dom/media/flac/FlacDecoder.cpp:46
(Diff revision 1)
> +{
> +  return IsEnabled() &&
> +    (aType.EqualsASCII("audio/flac") || aType.EqualsASCII("audio/x-flac") ||
> +     aType.EqualsASCII("application/x-flac"));
> +
> +  return false;

You can do with out this return false;

Comment 37

[u480271]

Comment on attachment 8782201 [details]
Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

https://reviewboard.mozilla.org/r/72410/#review70074

Comment 38

[u480271]

Comment on attachment 8782202 [details]
Bug 1195723: [flac] P10. Add media sniffer for flac file.

https://reviewboard.mozilla.org/r/72412/#review70078

Comment 39

[u480271]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

https://reviewboard.mozilla.org/r/72414/#review70080

Comment 40

[u480271]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

https://reviewboard.mozilla.org/r/72414/#review70082

Comment 41

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P12. Add sniffer for streaming flac.

Review commit: https://reviewboard.mozilla.org/r/72518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72518/

Comment 42

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P13. Add mochitest files.

Review commit: https://reviewboard.mozilla.org/r/72520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72520/

Comment 43

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P14. Add support for metadata.

Review commit: https://reviewboard.mozilla.org/r/72522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72522/

Comment 44

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: [flac] P15. Add metadata mochitest.

Review commit: https://reviewboard.mozilla.org/r/72524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72524/

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71018/diff/2-3/

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780309 [details]
Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71020/diff/2-3/

Comment 47

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71022/diff/2-3/

Comment 48

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782196 [details]
Bug 1195723: [adts] P4. Add extra mimetype for adts/aac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72400/diff/1-2/

Comment 49

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782197 [details]
Bug 1195723: P5. Add BitReader class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72402/diff/1-2/

Comment 50

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782198 [details]
Bug 1195723: [flac] P6. Add support for raw flac metadata decoding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72404/diff/1-2/

Comment 51

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782199 [details]
Bug 1195723: [flac] P7. Add flac demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72406/diff/1-2/

Comment 52

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72408/diff/1-2/

Comment 53

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782201 [details]
Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72410/diff/1-2/

Comment 54

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782202 [details]
Bug 1195723: [flac] P10. Add media sniffer for flac file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72412/diff/1-2/

Comment 55

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72414/diff/1-2/

Comment 56

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

https://reviewboard.mozilla.org/r/72408/#review70072

> You can do with out this return false;

also disabled flac on android for now. we don't compile ffvpx on this platform.

Comment 57

[u480271]

Comment on attachment 8782308 [details]
Bug 1195723: [flac] P12. Add sniffer for streaming flac.

https://reviewboard.mozilla.org/r/72518/#review70128

Comment 58

[u480271]

Comment on attachment 8782309 [details]
Bug 1195723: [flac] P13. Add mochitest files.

https://reviewboard.mozilla.org/r/72520/#review70130

Comment 59

[u480271]

Comment on attachment 8782310 [details]
Bug 1195723: [flac] P14. Add support for metadata.

https://reviewboard.mozilla.org/r/72522/#review70132

::: dom/media/flac/FlacDemuxer.cpp:617
(Diff revision 1)
>  UniquePtr<TrackInfo>
>  FlacTrackDemuxer::GetInfo() const
>  {
>    if (mParser->Info().IsValid()) {
>      // We have a proper metadata header.
> -    return mParser->Info().Clone();
> +    auto info = mParser->Info().Clone();

I don't think that's a great use of auto, since it's not clear what type it should be from the function call.

Comment 60

[u480271]

Comment on attachment 8782311 [details]
Bug 1195723: [flac] P15. Add metadata mochitest.

https://reviewboard.mozilla.org/r/72524/#review70134

Comment 61

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71018/diff/3-4/

Comment 62

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780309 [details]
Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71020/diff/3-4/

Comment 63

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71022/diff/3-4/

Comment 64

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782196 [details]
Bug 1195723: [adts] P4. Add extra mimetype for adts/aac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72400/diff/2-3/

Comment 65

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782197 [details]
Bug 1195723: P5. Add BitReader class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72402/diff/2-3/

Comment 66

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782198 [details]
Bug 1195723: [flac] P6. Add support for raw flac metadata decoding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72404/diff/2-3/

Comment 67

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782199 [details]
Bug 1195723: [flac] P7. Add flac demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72406/diff/2-3/

Comment 68

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72408/diff/2-3/

Comment 69

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782201 [details]
Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72410/diff/2-3/

Comment 70

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782202 [details]
Bug 1195723: [flac] P10. Add media sniffer for flac file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72412/diff/2-3/

Comment 71

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72414/diff/2-3/

Comment 72

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782308 [details]
Bug 1195723: [flac] P12. Add sniffer for streaming flac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72518/diff/1-2/

Comment 73

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782309 [details]
Bug 1195723: [flac] P13. Add mochitest files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72520/diff/1-2/

Comment 74

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782310 [details]
Bug 1195723: [flac] P14. Add support for metadata.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72522/diff/1-2/

Comment 75

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782311 [details]
Bug 1195723: [flac] P15. Add metadata mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72524/diff/1-2/

Comment 76

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782199 [details]
Bug 1195723: [flac] P7. Add flac demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72406/diff/3-4/

Comment 77

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72408/diff/3-4/

Comment 78

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782201 [details]
Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72410/diff/3-4/

Comment 79

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782202 [details]
Bug 1195723: [flac] P10. Add media sniffer for flac file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72412/diff/3-4/

Comment 80

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72414/diff/3-4/

Comment 81

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782308 [details]
Bug 1195723: [flac] P12. Add sniffer for streaming flac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72518/diff/2-3/

Comment 82

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782309 [details]
Bug 1195723: [flac] P13. Add mochitest files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72520/diff/2-3/

Comment 83

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782310 [details]
Bug 1195723: [flac] P14. Add support for metadata.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72522/diff/2-3/

Comment 84

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782311 [details]
Bug 1195723: [flac] P15. Add metadata mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72524/diff/2-3/

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04eceefa54c7
[flac] P1. Primitive flac frame parser. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/efb6bcf743e7
[ogg] P2. Refactor mimetype parsing for ogg. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/7f86beff4263
[ogg/flac] P3. Add flac support in ogg. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/a4f7a73c04fa
[adts] P4. Add extra mimetype for adts/aac. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/7bdb9540b60f
P5. Add BitReader class. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/c938ad9dd11c
[flac] P6. Add support for raw flac metadata decoding. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/867d6507a381
[flac] P7. Add flac demuxer. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/3fcb5682049a
[flac] P8. Add flac MediaDecoder. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/39908ea4c390
[flac] P9. Hook up flac decoder and demuxer. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/c2664064fa55
[flac] P10. Add media sniffer for flac file. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/4637f8be98bb
[flac] P11. Enable FLAC in Ogg by default. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/894061e6037e
[flac] P12. Add sniffer for streaming flac. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/ba0aeb71c672
[flac] P13. Add mochitest files. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/c3775a6225d8
[flac] P14. Add support for metadata. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/2a3b10281c9c
[flac] P15. Add metadata mochitest. r=kamidphish

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04eceefa54c7
https://hg.mozilla.org/mozilla-central/rev/efb6bcf743e7
https://hg.mozilla.org/mozilla-central/rev/7f86beff4263
https://hg.mozilla.org/mozilla-central/rev/a4f7a73c04fa
https://hg.mozilla.org/mozilla-central/rev/7bdb9540b60f
https://hg.mozilla.org/mozilla-central/rev/c938ad9dd11c
https://hg.mozilla.org/mozilla-central/rev/867d6507a381
https://hg.mozilla.org/mozilla-central/rev/3fcb5682049a
https://hg.mozilla.org/mozilla-central/rev/39908ea4c390
https://hg.mozilla.org/mozilla-central/rev/c2664064fa55
https://hg.mozilla.org/mozilla-central/rev/4637f8be98bb
https://hg.mozilla.org/mozilla-central/rev/894061e6037e
https://hg.mozilla.org/mozilla-central/rev/ba0aeb71c672
https://hg.mozilla.org/mozilla-central/rev/c3775a6225d8
https://hg.mozilla.org/mozilla-central/rev/2a3b10281c9c

Comment 87

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these (and bug 1270016) out for various media test failures and/or unexpected passes:
https://treeherder.mozilla.org/logviewer.html#?job_id=2282261&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=2279990&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/ecd29e0a2632

Comment 88

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1195723: P16. Adjust expected results.

The test uses either a theora/flac file or a h264/mp4 one.
XP doesn't have a h264 decoder and as such the test would have failed in
the past having no file to work with.

This is no longer the case now that flac in ogg is usable.

Review commit: https://reviewboard.mozilla.org/r/73426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73426/

Comment 89

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780308 [details]
Bug 1195723: [flac] P1. Primitive flac frame parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71018/diff/4-5/

Comment 90

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780309 [details]
Bug 1195723: [ogg] P2. Refactor mimetype parsing for ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71020/diff/4-5/

Comment 91

[Jean-Yves Avenard [:jya]]

Comment on attachment 8780310 [details]
Bug 1195723: [ogg/flac] P3. Add flac support in ogg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71022/diff/4-5/

Comment 92

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782196 [details]
Bug 1195723: [adts] P4. Add extra mimetype for adts/aac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72400/diff/3-4/

Comment 93

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782197 [details]
Bug 1195723: P5. Add BitReader class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72402/diff/3-4/

Comment 94

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782198 [details]
Bug 1195723: [flac] P6. Add support for raw flac metadata decoding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72404/diff/3-4/

Comment 95

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782199 [details]
Bug 1195723: [flac] P7. Add flac demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72406/diff/4-5/

Comment 96

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782200 [details]
Bug 1195723: [flac] P8. Add flac MediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72408/diff/4-5/

Comment 97

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782201 [details]
Bug 1195723: [flac] P9. Hook up flac decoder and demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72410/diff/4-5/

Comment 98

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782202 [details]
Bug 1195723: [flac] P10. Add media sniffer for flac file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72412/diff/4-5/

Comment 99

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782203 [details]
Bug 1195723: [flac] P11. Enable FLAC in Ogg by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72414/diff/4-5/

Comment 100

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782308 [details]
Bug 1195723: [flac] P12. Add sniffer for streaming flac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72518/diff/3-4/

Comment 101

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782309 [details]
Bug 1195723: [flac] P13. Add mochitest files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72520/diff/3-4/

Comment 102

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782310 [details]
Bug 1195723: [flac] P14. Add support for metadata.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72522/diff/3-4/

Comment 103

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782311 [details]
Bug 1195723: [flac] P15. Add metadata mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72524/diff/3-4/

Comment 105

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8783735 [details]
Bug 1195723: P16. Adjust expected results.

https://reviewboard.mozilla.org/r/73426/#review71258

Comment 106

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d12c7bc5be51
[flac] P1. Primitive flac frame parser. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/1a6c05bb21f0
[ogg] P2. Refactor mimetype parsing for ogg. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/a95820f09bdd
[ogg/flac] P3. Add flac support in ogg. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/a2d5451f56bc
[adts] P4. Add extra mimetype for adts/aac. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/6feb50a80c50
P5. Add BitReader class. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/d8a7f672b3f4
[flac] P6. Add support for raw flac metadata decoding. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/6f02e9165d09
[flac] P7. Add flac demuxer. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/2a136bde12bd
[flac] P8. Add flac MediaDecoder. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/a25add7d4afc
[flac] P9. Hook up flac decoder and demuxer. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/1ce419e3955f
[flac] P10. Add media sniffer for flac file. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/01ff85a264b9
[flac] P11. Enable FLAC in Ogg by default. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/e42414eeeabe
[flac] P12. Add sniffer for streaming flac. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/53c739399220
[flac] P13. Add mochitest files. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/612d774f82ee
[flac] P14. Add support for metadata. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/2ae43363f27a
[flac] P15. Add metadata mochitest. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/8d448381edbb
P16. Adjust expected results. r=gerald

Comment 107

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d12c7bc5be51
https://hg.mozilla.org/mozilla-central/rev/1a6c05bb21f0
https://hg.mozilla.org/mozilla-central/rev/a95820f09bdd
https://hg.mozilla.org/mozilla-central/rev/a2d5451f56bc
https://hg.mozilla.org/mozilla-central/rev/6feb50a80c50
https://hg.mozilla.org/mozilla-central/rev/d8a7f672b3f4
https://hg.mozilla.org/mozilla-central/rev/6f02e9165d09
https://hg.mozilla.org/mozilla-central/rev/2a136bde12bd
https://hg.mozilla.org/mozilla-central/rev/a25add7d4afc
https://hg.mozilla.org/mozilla-central/rev/1ce419e3955f
https://hg.mozilla.org/mozilla-central/rev/01ff85a264b9
https://hg.mozilla.org/mozilla-central/rev/e42414eeeabe
https://hg.mozilla.org/mozilla-central/rev/53c739399220
https://hg.mozilla.org/mozilla-central/rev/612d774f82ee
https://hg.mozilla.org/mozilla-central/rev/2ae43363f27a
https://hg.mozilla.org/mozilla-central/rev/8d448381edbb

Comment 111

[Chris Peterson [:cpeterson]]

jya, should we mention FLAC support in our Firefox 51 release notes? Or is there more work that must be completed before FLAC is usable by web content?

Comment 112

[Jean-Yves Avenard [:jya]]

Support is complete as far as playing plain flac goes. 

Obviously being so new and written from scratch I'm expecting issues. 

Feature wise, supporting chaining of flac similar to how we do it with Ogg could be interesting. But seeing that no one support flac streaming (they always do so in Ogg) it may not matter. 

So yes, it can be mentioned in the release notes.

Comment 113

[Chris Peterson [:cpeterson]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Firefox is adding support for another audio codec.
[Suggested wording]: Added support for FLAC (Free Lossless Audio Codec) playback.
[Links (documentation, blog post, etc)]: https://xiph.org/flac/

We should probably add a note about FLAC support on MDN, so I'm adding the dev-doc-needed keyword to this bug.

Comment 114

[Andrei Vaida [:avaida]]

This looks like something manual QA could help test. jya, what do you think? Should we treat this as a full blown feature or just add a few tests to our audio compatibility test suites?

Comment 115

[Jean-Yves Avenard [:jya]]

Feel free to add any tests you see fit. Area of testing is seeking and if it plays to the end. 

At this stage, everything should work. I'm not aware of anything not working or supported.

Comment 116

[Jean-Yves Avenard [:jya]]

(In reply to mattia.b89 from comment #6)
> I confirm FLAC doesn't work
> 
> I comment in order to share with you an easy test case:
> http://hpr.dogphilosophy.net/test/

You may want to try that with current Nightly. You'll find that it will work just fine.

Comment 117

[mattia.b89]

It works!
Great work!

PS: with nightly version (51) I noticed new high-res icon, mandatory for my monitor

Comment 118

[Gerry Chang [:gchang]]

Added to Fx51 (Aurora) release notes.

Comment 119

[Ciprian Georgiu, Desktop QA]

[Test Plan]:
https://wiki.mozilla.org/QA/FLAC_support

Comment 120

[Eric Shepherd [:sheppy]]

Docs are up-to-date already; someone beat me to it. Nice job. :)

Comment 121

[Jean-Yves Avenard [:jya]]

I quickly filled up the MDN docs. Not sure if that's the docs you referred to

Comment 122

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on 51.0b2-build1 (2016-11-21) across platforms:
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 16.04 x64 LTS

Comment 123

[Andrew Valencia]

> also disabled flac on android for now. we don't compile ffvpx on this platform.

While any start to flac support is welcome, I hope you will reconsider omitting this support
from Android.  We are rapidly approaching the point where, to a first approximation, if you
don't support mobile web, you don't support the web.

Comment 124

[Gerry Chang [:gchang]]

In the release notes of 51 with "Added support for FLAC (Free Lossless Audio Codec) playback" as wording.