| Message ID | 20221031160915.673782-1-mvanb1@gmail.com |
|---|---|
| State | Accepted |
| Commit | bf6c500cff88564f3e47d8deb88bd3cd3526f812 |
| Headers | show |
| Series | [FFmpeg-devel] libavcodec/flacenc: Enable sample rates > 655350 Hz | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 10/31/2022 4:09 PM, Martijn van Beurden wrote: > Also, make use of the full sample rate code table > --- > libavcodec/flacenc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) It is interesting that the IETF RFC and the Xiph spec disagree on whether this is allowed. The Xiph spec says: Sample rate in Hz. Though 20 bits are available, the maximum sample rate is limited by the structure of frame headers to 655350Hz. Also, a value of 0 is invalid. The RFC just says: Sample rate in Hz. I see your name on the RFC - can you provide some insight here? - Derek
Op ma 31 okt. 2022 om 17:58 schreef Derek Buitenhuis <derek.buitenhuis@gmail.com>: > > It is interesting that the IETF RFC and the Xiph spec disagree on whether this is allowed. > The Xiph spec also says the IETF spec is better, and it remains as historical reference and overview :) > The Xiph spec says: > > Sample rate in Hz. Though 20 bits are available, the maximum sample rate is limited by the > structure of frame headers to 655350Hz. Also, a value of 0 is invalid. > > The RFC just says: > > Sample rate in Hz. > The spec as it is on the FLAC website (which is being "preserved") is wrong. I don't know how this came to be, I think it was at first poorly worded and later incorrectly fixed. See this commit: https://github.com/xiph/flac/commit/96534bb5f35eb9c2f6f393dc470625e9c74df1a5 The text as it was before that commit doesn't make any sense, the text as it is after the commit is not correct either. The issue here is that FLAC has a sample rate in the streaminfo metadata block, at the very start of the file. That one can accommodate sample rates up to 2^20-1. The frame headers repeat the sample rate every frame and can only accommodate up to 655350Hz, but they can also reference the streaminfo metadata block. Because of the possibility to reference that 20 bit number, it is possible to store sample rates up to 1048575Hz. You can see this patch only touches the encoder: the FFmpeg decoder has already been equipped to deal with this since its inception in 2004. There is some kind of limitation to sample rates above 655350Hz, or samplerates between 65535Hz and 655350Hz that are not a multiple of 10 though: a FLAC file with such a sample rate cannot be multicast, because a decoder receiving a multicast stream does not receive the streaminfo metadata block, and thus cannot use it to figure out the correct sample rate. Please let me know when this explanation falls short. Kind regards, Martijn van Beurden
On 10/31/2022 6:33 PM, Martijn van Beurden wrote: > The Xiph spec also says the IETF spec is better, and it remains as > historical reference and overview :) So it does. > The spec as it is on the FLAC website (which is being "preserved") is > wrong. I don't know how this came to be, I think it was at first > poorly worded and later incorrectly fixed. See this commit: > https://github.com/xiph/flac/commit/96534bb5f35eb9c2f6f393dc470625e9c74df1a5 > The text as it was before that commit doesn't make any sense, the text > as it is after the commit is not correct either. Well that's pretty confusing, indeed. > The issue here is that FLAC has a sample rate in the streaminfo > metadata block, at the very start of the file. That one can > accommodate sample rates up to 2^20-1. The frame headers repeat the > sample rate every frame and can only accommodate up to 655350Hz, but > they can also reference the streaminfo metadata block. Because of the > possibility to reference that 20 bit number, it is possible to store > sample rates up to 1048575Hz. You can see this patch only touches the > encoder: the FFmpeg decoder has already been equipped to deal with > this since its inception in 2004. > > There is some kind of limitation to sample rates above 655350Hz, or > samplerates between 65535Hz and 655350Hz that are not a multiple of 10 > though: a FLAC file with such a sample rate cannot be multicast, > because a decoder receiving a multicast stream does not receive the > streaminfo metadata block, and thus cannot use it to figure out the > correct sample rate. > > Please let me know when this explanation falls short. It all makes sense, thanks for the explanation. - Derek
Op ma 31 okt. 2022 om 19:33 schreef Martijn van Beurden <mvanb1@gmail.com>: > > Op ma 31 okt. 2022 om 17:58 schreef Derek Buitenhuis > <derek.buitenhuis@gmail.com>: > > > > It is interesting that the IETF RFC and the Xiph spec disagree on whether this is allowed. > > > > The Xiph spec also says the IETF spec is better, and it remains as > historical reference and overview :) > > > The Xiph spec says: > > > > Sample rate in Hz. Though 20 bits are available, the maximum sample rate is limited by the > > structure of frame headers to 655350Hz. Also, a value of 0 is invalid. > > > > The RFC just says: > > > > Sample rate in Hz. > > > > The spec as it is on the FLAC website (which is being "preserved") is > wrong. I don't know how this came to be, I think it was at first > poorly worded and later incorrectly fixed. See this commit: > https://github.com/xiph/flac/commit/96534bb5f35eb9c2f6f393dc470625e9c74df1a5 > The text as it was before that commit doesn't make any sense, the text > as it is after the commit is not correct either. > > The issue here is that FLAC has a sample rate in the streaminfo > metadata block, at the very start of the file. That one can > accommodate sample rates up to 2^20-1. The frame headers repeat the > sample rate every frame and can only accommodate up to 655350Hz, but > they can also reference the streaminfo metadata block. Because of the > possibility to reference that 20 bit number, it is possible to store > sample rates up to 1048575Hz. You can see this patch only touches the > encoder: the FFmpeg decoder has already been equipped to deal with > this since its inception in 2004. > > There is some kind of limitation to sample rates above 655350Hz, or > samplerates between 65535Hz and 655350Hz that are not a multiple of 10 > though: a FLAC file with such a sample rate cannot be multicast, > because a decoder receiving a multicast stream does not receive the > streaminfo metadata block, and thus cannot use it to figure out the > correct sample rate. > > Please let me know when this explanation falls short. > > Kind regards, Martijn van Beurden Hi all, With this email, I would like to renew the attention of the mailing list for this patch. Kind regards, Martijn van Beurden
On Tue, Nov 15, 2022 at 05:14:26PM +0100, Martijn van Beurden wrote: > Op ma 31 okt. 2022 om 19:33 schreef Martijn van Beurden <mvanb1@gmail.com>: > > > > Op ma 31 okt. 2022 om 17:58 schreef Derek Buitenhuis > > <derek.buitenhuis@gmail.com>: > > > > > > It is interesting that the IETF RFC and the Xiph spec disagree on whether this is allowed. > > > > > > > The Xiph spec also says the IETF spec is better, and it remains as > > historical reference and overview :) > > > > > The Xiph spec says: > > > > > > Sample rate in Hz. Though 20 bits are available, the maximum sample rate is limited by the > > > structure of frame headers to 655350Hz. Also, a value of 0 is invalid. > > > > > > The RFC just says: > > > > > > Sample rate in Hz. > > > > > > > The spec as it is on the FLAC website (which is being "preserved") is > > wrong. I don't know how this came to be, I think it was at first > > poorly worded and later incorrectly fixed. See this commit: > > https://github.com/xiph/flac/commit/96534bb5f35eb9c2f6f393dc470625e9c74df1a5 > > The text as it was before that commit doesn't make any sense, the text > > as it is after the commit is not correct either. > > > > The issue here is that FLAC has a sample rate in the streaminfo > > metadata block, at the very start of the file. That one can > > accommodate sample rates up to 2^20-1. The frame headers repeat the > > sample rate every frame and can only accommodate up to 655350Hz, but > > they can also reference the streaminfo metadata block. Because of the > > possibility to reference that 20 bit number, it is possible to store > > sample rates up to 1048575Hz. You can see this patch only touches the > > encoder: the FFmpeg decoder has already been equipped to deal with > > this since its inception in 2004. > > > > There is some kind of limitation to sample rates above 655350Hz, or > > samplerates between 65535Hz and 655350Hz that are not a multiple of 10 > > though: a FLAC file with such a sample rate cannot be multicast, > > because a decoder receiving a multicast stream does not receive the > > streaminfo metadata block, and thus cannot use it to figure out the > > correct sample rate. > > > > Please let me know when this explanation falls short. > > > > Kind regards, Martijn van Beurden > > Hi all, > > With this email, I would like to renew the attention of the mailing > list for this patch. will apply thx [...]
diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index 5d8c3f82be..bca71b3780 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -299,7 +299,7 @@ static av_cold int flac_encode_init(AVCodecContext *avctx) /* find samplerate in table */ if (freq < 1) return AVERROR(EINVAL); - for (i = 4; i < 12; i++) { + for (i = 1; i < 12; i++) { if (freq == ff_flac_sample_rate_table[i]) { s->samplerate = ff_flac_sample_rate_table[i]; s->sr_code[0] = i; @@ -318,6 +318,9 @@ static av_cold int flac_encode_init(AVCodecContext *avctx) } else if (freq < 65535) { s->sr_code[0] = 13; s->sr_code[1] = freq; + } else if (freq < 1048576) { + s->sr_code[0] = 0; + s->sr_code[1] = 0; } else { av_log(avctx, AV_LOG_ERROR, "%d Hz not supported\n", freq); return AVERROR(EINVAL);