1270016 - Create FLAC MediaDataDecoder

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Jean-Yves Avenard [:jya]]

We need this to be able to play flac (either native container or contained in ogg)

Comment 1

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

Mass change P2 -> P3

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

Code was extracted from FFmpeg 3.1.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 4

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

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

https://reviewboard.mozilla.org/r/68560/#review65806

r+ with the sign inversion problem fixed.

::: dom/media/AudioSampleFormat.h:87
(Diff revision 1)
>  }
> +inline float
> +AudioSampleToFloat(int32_t aValue)
> +{
> +  return aValue/(float)(1<<31);
> +}

1 << 31 is negative - see below:

ajones@DESKTOP-IELR441:~$ cat test.c
#include <stdio.h>

int main(int argc, char* argv[])
{
  printf("%f\n", (float)(1 << 31));
  return 0;
}
ajones@DESKTOP-IELR441:~$ clang-3.6 test.c
ajones@DESKTOP-IELR441:~$ ./a.out
-2147483648.000000
ajones@DESKTOP-IELR441:~$

Comment 5

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

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

https://reviewboard.mozilla.org/r/68558/#review65768

Comment 6

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/68560/#review65806

> 1 << 31 is negative - see below:
> 
> ajones@DESKTOP-IELR441:~$ cat test.c
> #include <stdio.h>
> 
> int main(int argc, char* argv[])
> {
>   printf("%f\n", (float)(1 << 31));
>   return 0;
> }
> ajones@DESKTOP-IELR441:~$ clang-3.6 test.c
> ajones@DESKTOP-IELR441:~$ ./a.out
> -2147483648.000000
> ajones@DESKTOP-IELR441:~$

would have sounded the same :)

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

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

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

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

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1270016: P3. Only check audio format at decode time.

With raw streaming flac, we can't determine the audio format at the time the codec is opened.
Additionally, when using streaming flac, we do not have a STREAMINFO block. The FFmpeg flac decoder check that extradata is null and not its size and would error due to an invalid extradata.

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

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

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

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 16

[u480271]

Comment on attachment 8781839 [details]
Bug 1270016: P3. Only check audio format at decode time.

https://reviewboard.mozilla.org/r/72164/#review70006

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

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

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8776925 [details]
Bug 1270016: P2. Make the FFVPX PDM work with FLAC.

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

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8781839 [details]
Bug 1270016: P3. Only check audio format at decode time.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a0f3231b43e
P1. Add FLAC support to our ffmpeg extract. r=kentuckyfriedtakahe
https://hg.mozilla.org/integration/autoland/rev/8359b182bb99
P2. Make the FFVPX PDM work with FLAC. r=kentuckyfriedtakahe
https://hg.mozilla.org/integration/autoland/rev/9178e6e14a5a
P3. Only check audio format at decode time. r=kamidphish

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a0f3231b43e
https://hg.mozilla.org/mozilla-central/rev/8359b182bb99
https://hg.mozilla.org/mozilla-central/rev/9178e6e14a5a

Comment 22

[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/e6b665f32057

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/204316468536
P1. Add FLAC support to our ffmpeg extract. r=kentuckyfriedtakahe
https://hg.mozilla.org/integration/autoland/rev/b23729f27e03
P2. Make the FFVPX PDM work with FLAC. r=kentuckyfriedtakahe
https://hg.mozilla.org/integration/autoland/rev/db0f179fdb85
P3. Only check audio format at decode time. r=kamidphish

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/204316468536
https://hg.mozilla.org/mozilla-central/rev/b23729f27e03
https://hg.mozilla.org/mozilla-central/rev/db0f179fdb85