Create FLAC MediaDataDecoder

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
We need this to be able to play flac (either native container or contained in ogg)
Priority: -- → P2
Mass change P2 -> P3
Priority: P2 → P3
(Assignee)

Updated

2 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 2

2 years ago
Created attachment 8776924 [details]
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/
Attachment #8776924 - Flags: review?(ajones)
Attachment #8776925 - Flags: review?(ajones)
(Assignee)

Comment 3

2 years ago
Created attachment 8776925 [details]
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 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:~$
Attachment #8776925 - Flags: review?(ajones) → review+
Comment on attachment 8776924 [details]
Bug 1270016: P1. Add FLAC support to our ffmpeg extract.

https://reviewboard.mozilla.org/r/68558/#review65768
Attachment #8776924 - Flags: review?(ajones) → review+
(Assignee)

Comment 6

2 years ago
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 :)
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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/
(Assignee)

Comment 9

2 years ago
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/
(Assignee)

Comment 10

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8781839 [details]
Bug 1270016: P3. Only check audio format at decode time.

https://reviewboard.mozilla.org/r/72164/#review70006
Attachment #8781839 - Flags: review?(dglastonbury) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
Pushed by jyavenard@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a0f3231b43e
https://hg.mozilla.org/mozilla-central/rev/8359b182bb99
https://hg.mozilla.org/mozilla-central/rev/9178e6e14a5a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Status: RESOLVED → REOPENED
status-firefox51: fixed → affected
Flags: needinfo?(jyavenard)
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---

Comment 23

2 years ago
Pushed by jyavenard@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/204316468536
https://hg.mozilla.org/mozilla-central/rev/b23729f27e03
https://hg.mozilla.org/mozilla-central/rev/db0f179fdb85
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

2 years ago
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.