Hi, This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. The rate control is not as intuitive to use as I would like, but it works. libspeex has the option to have the library choose a CBR bitrate based on a quality setting. Providing that option doesn't really fit well into our current system since there is no way to tell if the user is specifying CBR quality or VBR quality. So instead it just uses bitrate for CBR and quality for VBR like our other audio encoders. The default bitrate of 64kbps is higher than the maximum Speex bitrate, so at least it will be good quality by default. Thanks, Justin
Justin Ruggles <justin.ruggles at gmail.com> writes:
Hi,
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke.
Makefile and configure bits are OK. -- M?ns Rullg?rd mans at mansr.com
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
--- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -31,7 +31,7 @@
#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 37 -#define LIBAVCODEC_VERSION_MICRO 1 +#define LIBAVCODEC_VERSION_MICRO 2
What is it with micro bumps all of a sudden? You need to bump minor. Diego
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
--- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -31,7 +31,7 @@
#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 37 -#define LIBAVCODEC_VERSION_MICRO 1 +#define LIBAVCODEC_VERSION_MICRO 2
What is it with micro bumps all of a sudden? You need to bump minor.
What exactly are the rules? -- M?ns Rullg?rd mans at mansr.com
On Mon, Oct 26, 2009 at 09:37:21AM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
--- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -31,7 +31,7 @@
#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 37 -#define LIBAVCODEC_VERSION_MICRO 1 +#define LIBAVCODEC_VERSION_MICRO 2
What is it with micro bumps all of a sudden? You need to bump minor.
What exactly are the rules?
Normally to my knowledge: micro: bug fixes, "100%" forward- and backward compatibility. minor: new features or API changes, particularly when code written specifically for the new version will not work with an older one. major: ABI or incompatible API changes (or for non-libraries major features)
On Mon, Oct 26, 2009 at 09:37:21AM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
--- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -31,7 +31,7 @@
#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 37 -#define LIBAVCODEC_VERSION_MICRO 1 +#define LIBAVCODEC_VERSION_MICRO 2
What is it with micro bumps all of a sudden? You need to bump minor.
What exactly are the rules?
The ones outlined in our development policy: http://www.ffmpeg.org/developer.html 1.4 Development Policy 16. Remember to check if you need to bump versions for the specific libav parts (libavutil, libavcodec, libavformat) you are changing. You need to change the version integer. Incrementing the first component means no backward compatibility to previous versions (e.g. removal of a function from the public API). Incrementing the second component means backward compatible change (e.g. addition of a function to the public API or extension of an existing data structure). Incrementing the third component means a noteworthy binary compatible change (e.g. encoder bug fix that matters for the decoder). 1.6 New codecs or formats checklist 3. Did you bump the minor version number in `avcodec.h' or `avformat.h'? For each new format the minor of the corresponding lib has to be bumped. Vitor seems to have managed to bend the rules in people's head by setting just one bad example... Diego
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
Hi,
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke.
The rate control is not as intuitive to use as I would like, but it works. libspeex has the option to have the library choose a CBR bitrate based on a quality setting. Providing that option doesn't really fit well into our current system since there is no way to tell if the user is specifying CBR quality or VBR quality. So instead it just uses bitrate for CBR and quality for VBR like our other audio encoders. The default bitrate of 64kbps is higher than the maximum Speex bitrate, so at least it will be good quality by default.
[...]
+static av_cold int encode_init(AVCodecContext *avctx) +{ + LibSpeexEncContext *s = avctx->priv_data; + const SpeexMode *mode; + uint8_t *header_data; + int header_size; + int32_t complexity; + + /* channels */ + if (avctx->channels < 1 || avctx->channels > 2) { + av_log(avctx, AV_LOG_ERROR, "Invalid channels (%d). Only stereo and " + "mono are supported\n", avctx->channels); + return -1; + } + + /* sample rate and encoding mode */ + switch (avctx->sample_rate) { + case 8000: mode = &speex_nb_mode; break; + case 16000: mode = &speex_wb_mode; break; + case 32000: mode = &speex_uwb_mode; break; + default: + av_log(avctx, AV_LOG_ERROR, "Sample rate of %d Hz is not supported. " + "Resample to 8, 16, or 32 kHz.\n", avctx->sample_rate); + return -1; + } + + /* initialize libspeex */ + s->enc_state = speex_encoder_init(mode); + if (!s->enc_state) { + av_log(avctx, AV_LOG_ERROR, "Error initializing libspeex\n"); + return -1; + } + speex_init_header(&s->header, avctx->sample_rate, avctx->channels, mode); + + /* rate control method and parameters */ + if (avctx->flags & CODEC_FLAG_QSCALE) { + /* VBR */ + s->header.vbr = 1; + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR, &s->header.vbr); + s->vbr_quality = av_clipf(avctx->global_quality / (float)FF_QP2LAMBDA, + 0.0f, 10.0f); + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR_QUALITY, &s->vbr_quality); + avctx->bit_rate = 0; + } else { + /* CBR */ + s->header.bitrate = avctx->bit_rate; + speex_encoder_ctl(s->enc_state, SPEEX_SET_BITRATE, &s->header.bitrate); + speex_encoder_ctl(s->enc_state, SPEEX_GET_BITRATE, &s->header.bitrate); + /* stereo side information adds about 800 bps to the base bitrate */
+ avctx->bit_rate = s->header.bitrate + (avctx->channels == 2 ? 800 : 0);
avctx->bit_rate is set by the user and not the encoder
+ } + + /* set encoding complexity */ + if (avctx->compression_level > FF_COMPRESSION_DEFAULT) { + complexity = av_clip(avctx->compression_level, 0, 10); + speex_encoder_ctl(s->enc_state, SPEEX_SET_COMPLEXITY, &complexity); + } + speex_encoder_ctl(s->enc_state, SPEEX_GET_COMPLEXITY, &complexity);
+ avctx->compression_level = complexity;
same [...]
+static int encode_frame(AVCodecContext *avctx, uint8_t *frame, int buf_size, + void *data) +{ + LibSpeexEncContext *s = avctx->priv_data; + void *samples = data; + int nframes, i; + + if (!avctx->frame_size) + return 0; + + /* handle last packet, which may have fewer frames-per-packet and/or + fewer samples in the last frame */ + nframes = s->header.frames_per_packet; + if (avctx->frame_size < nframes * s->header.frame_size) { + nframes = (avctx->frame_size + s->header.frame_size - 1) / + s->header.frame_size; + if (avctx->frame_size != s->header.frame_size * nframes) { + /* allocate new buffer to pad last frame */ + int new_samples_size;
+ avctx->frame_size = nframes * s->header.frame_size;
iam not sure if this violates the API but at least i would say it is unexpected by the application
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size);
i think the application is or at least should be required to allocate full frames even for the possibly smaller last
+ } + } + + /* encode Speex frames */ + speex_bits_reset(&s->bits); + if (avctx->sample_fmt == SAMPLE_FMT_FLT) { + float *samples_flt = samples; +
+ /* scale floating point samples to 16-bit range as required by libspeex */ + if (avctx->sample_fmt == SAMPLE_FMT_FLT) + for (i = 0; i < avctx->frame_size * avctx->channels; i++) + samples_flt[i] *= 32767.0;
the audio encode function is: int avcodec_encode_audio(AVCodecContext *avctx, uint8_t *buf, int buf_size, const short *samples); samples is const [...]
+AVCodec libspeex_encoder = { + "libspeex", + CODEC_TYPE_AUDIO, + CODEC_ID_SPEEX, + sizeof(LibSpeexEncContext), + encode_init, + encode_frame, + encode_close, + NULL, + .capabilities = CODEC_CAP_SMALL_LAST_FRAME, + .sample_fmts = (const enum SampleFormat[]){ SAMPLE_FMT_S16, SAMPLE_FMT_FLT, + SAMPLE_FMT_NONE }, + .long_name = NULL_IF_CONFIG_SMALL("libspeex Speex"),
missing supported_samplerates and channel_layouts [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
Michael Niedermayer wrote:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
Hi,
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke.
The rate control is not as intuitive to use as I would like, but it works. libspeex has the option to have the library choose a CBR bitrate based on a quality setting. Providing that option doesn't really fit well into our current system since there is no way to tell if the user is specifying CBR quality or VBR quality. So instead it just uses bitrate for CBR and quality for VBR like our other audio encoders. The default bitrate of 64kbps is higher than the maximum Speex bitrate, so at least it will be good quality by default.
[...]
+static av_cold int encode_init(AVCodecContext *avctx) +{ + LibSpeexEncContext *s = avctx->priv_data; + const SpeexMode *mode; + uint8_t *header_data; + int header_size; + int32_t complexity; + + /* channels */ + if (avctx->channels < 1 || avctx->channels > 2) { + av_log(avctx, AV_LOG_ERROR, "Invalid channels (%d). Only stereo and " + "mono are supported\n", avctx->channels); + return -1; + } + + /* sample rate and encoding mode */ + switch (avctx->sample_rate) { + case 8000: mode = &speex_nb_mode; break; + case 16000: mode = &speex_wb_mode; break; + case 32000: mode = &speex_uwb_mode; break; + default: + av_log(avctx, AV_LOG_ERROR, "Sample rate of %d Hz is not supported. " + "Resample to 8, 16, or 32 kHz.\n", avctx->sample_rate); + return -1; + } + + /* initialize libspeex */ + s->enc_state = speex_encoder_init(mode); + if (!s->enc_state) { + av_log(avctx, AV_LOG_ERROR, "Error initializing libspeex\n"); + return -1; + } + speex_init_header(&s->header, avctx->sample_rate, avctx->channels, mode); + + /* rate control method and parameters */ + if (avctx->flags & CODEC_FLAG_QSCALE) { + /* VBR */ + s->header.vbr = 1; + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR, &s->header.vbr); + s->vbr_quality = av_clipf(avctx->global_quality / (float)FF_QP2LAMBDA, + 0.0f, 10.0f); + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR_QUALITY, &s->vbr_quality); + avctx->bit_rate = 0; + } else { + /* CBR */ + s->header.bitrate = avctx->bit_rate; + speex_encoder_ctl(s->enc_state, SPEEX_SET_BITRATE, &s->header.bitrate); + speex_encoder_ctl(s->enc_state, SPEEX_GET_BITRATE, &s->header.bitrate); + /* stereo side information adds about 800 bps to the base bitrate */
+ avctx->bit_rate = s->header.bitrate + (avctx->channels == 2 ? 800 : 0);
avctx->bit_rate is set by the user and not the encoder
The reason for this is for feedback to the user. libspeex uses the closest supported bitrate for the selected mode that is less than or equal to the requested bitrate. I guess this could be taken out and just let libspeex do whatever without telling the user except maybe in a debug printout. The bitrate is a nominal value anyway. The exact bitrate also depends on the number of frames per packet because frames are not byte aligned, packets are.
+ } + + /* set encoding complexity */ + if (avctx->compression_level > FF_COMPRESSION_DEFAULT) { + complexity = av_clip(avctx->compression_level, 0, 10); + speex_encoder_ctl(s->enc_state, SPEEX_SET_COMPLEXITY, &complexity); + } + speex_encoder_ctl(s->enc_state, SPEEX_GET_COMPLEXITY, &complexity);
+ avctx->compression_level = complexity;
same
Yeah, I guess this could be taken out without much difference.
[...]
+static int encode_frame(AVCodecContext *avctx, uint8_t *frame, int buf_size, + void *data) +{ + LibSpeexEncContext *s = avctx->priv_data; + void *samples = data; + int nframes, i; + + if (!avctx->frame_size) + return 0; + + /* handle last packet, which may have fewer frames-per-packet and/or + fewer samples in the last frame */ + nframes = s->header.frames_per_packet; + if (avctx->frame_size < nframes * s->header.frame_size) { + nframes = (avctx->frame_size + s->header.frame_size - 1) / + s->header.frame_size; + if (avctx->frame_size != s->header.frame_size * nframes) { + /* allocate new buffer to pad last frame */ + int new_samples_size;
+ avctx->frame_size = nframes * s->header.frame_size;
iam not sure if this violates the API but at least i would say it is unexpected by the application
Hmmm. Yeah, if it doesn't violate API, it is at least not documented. Is there another way to report the correct duration of the output frame if the user gives, for example, 500 samples and the output frame represents 640 due to padding?
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size);
i think the application is or at least should be required to allocate full frames even for the possibly smaller last
Where should this be documented? avcodec_encode_audio()?
+ } + } + + /* encode Speex frames */ + speex_bits_reset(&s->bits); + if (avctx->sample_fmt == SAMPLE_FMT_FLT) { + float *samples_flt = samples; +
+ /* scale floating point samples to 16-bit range as required by libspeex */ + if (avctx->sample_fmt == SAMPLE_FMT_FLT) + for (i = 0; i < avctx->frame_size * avctx->channels; i++) + samples_flt[i] *= 32767.0;
the audio encode function is: int avcodec_encode_audio(AVCodecContext *avctx, uint8_t *buf, int buf_size, const short *samples); samples is const
ouch. guess I'll have to alloc and memcpy for float samples.
[...]
+AVCodec libspeex_encoder = { + "libspeex", + CODEC_TYPE_AUDIO, + CODEC_ID_SPEEX, + sizeof(LibSpeexEncContext), + encode_init, + encode_frame, + encode_close, + NULL, + .capabilities = CODEC_CAP_SMALL_LAST_FRAME, + .sample_fmts = (const enum SampleFormat[]){ SAMPLE_FMT_S16, SAMPLE_FMT_FLT, + SAMPLE_FMT_NONE }, + .long_name = NULL_IF_CONFIG_SMALL("libspeex Speex"),
missing supported_samplerates and channel_layouts
ah, forgot about that. i'll fix it. Thanks, Justin
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
Hi,
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke.
The rate control is not as intuitive to use as I would like, but it works. libspeex has the option to have the library choose a CBR bitrate based on a quality setting. Providing that option doesn't really fit well into our current system since there is no way to tell if the user is specifying CBR quality or VBR quality. So instead it just uses bitrate for CBR and quality for VBR like our other audio encoders. The default bitrate of 64kbps is higher than the maximum Speex bitrate, so at least it will be good quality by default.
[...]
+static av_cold int encode_init(AVCodecContext *avctx) +{ + LibSpeexEncContext *s = avctx->priv_data; + const SpeexMode *mode; + uint8_t *header_data; + int header_size; + int32_t complexity; + + /* channels */ + if (avctx->channels < 1 || avctx->channels > 2) { + av_log(avctx, AV_LOG_ERROR, "Invalid channels (%d). Only stereo and " + "mono are supported\n", avctx->channels); + return -1; + } + + /* sample rate and encoding mode */ + switch (avctx->sample_rate) { + case 8000: mode = &speex_nb_mode; break; + case 16000: mode = &speex_wb_mode; break; + case 32000: mode = &speex_uwb_mode; break; + default: + av_log(avctx, AV_LOG_ERROR, "Sample rate of %d Hz is not supported. " + "Resample to 8, 16, or 32 kHz.\n", avctx->sample_rate); + return -1; + } + + /* initialize libspeex */ + s->enc_state = speex_encoder_init(mode); + if (!s->enc_state) { + av_log(avctx, AV_LOG_ERROR, "Error initializing libspeex\n"); + return -1; + } + speex_init_header(&s->header, avctx->sample_rate, avctx->channels, mode); + + /* rate control method and parameters */ + if (avctx->flags & CODEC_FLAG_QSCALE) { + /* VBR */ + s->header.vbr = 1; + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR, &s->header.vbr); + s->vbr_quality = av_clipf(avctx->global_quality / (float)FF_QP2LAMBDA, + 0.0f, 10.0f); + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR_QUALITY, &s->vbr_quality); + avctx->bit_rate = 0; + } else { + /* CBR */ + s->header.bitrate = avctx->bit_rate; + speex_encoder_ctl(s->enc_state, SPEEX_SET_BITRATE, &s->header.bitrate); + speex_encoder_ctl(s->enc_state, SPEEX_GET_BITRATE, &s->header.bitrate); + /* stereo side information adds about 800 bps to the base bitrate */
+ avctx->bit_rate = s->header.bitrate + (avctx->channels == 2 ? 800 : 0);
avctx->bit_rate is set by the user and not the encoder
The reason for this is for feedback to the user. libspeex uses the closest supported bitrate for the selected mode that is less than or equal to the requested bitrate. I guess this could be taken out and just let libspeex do whatever without telling the user except maybe in a debug printout. The bitrate is a nominal value anyway. The exact bitrate also depends on the number of frames per packet because frames are not byte aligned, packets are.
hmm, ive no good comment/idea ATM :( [...]
[...]
+static int encode_frame(AVCodecContext *avctx, uint8_t *frame, int buf_size, + void *data) +{ + LibSpeexEncContext *s = avctx->priv_data; + void *samples = data; + int nframes, i; + + if (!avctx->frame_size) + return 0; + + /* handle last packet, which may have fewer frames-per-packet and/or + fewer samples in the last frame */ + nframes = s->header.frames_per_packet; + if (avctx->frame_size < nframes * s->header.frame_size) { + nframes = (avctx->frame_size + s->header.frame_size - 1) / + s->header.frame_size; + if (avctx->frame_size != s->header.frame_size * nframes) { + /* allocate new buffer to pad last frame */ + int new_samples_size;
+ avctx->frame_size = nframes * s->header.frame_size;
iam not sure if this violates the API but at least i would say it is unexpected by the application
Hmmm. Yeah, if it doesn't violate API, it is at least not documented. Is there another way to report the correct duration of the output frame if the user gives, for example, 500 samples and the output frame represents 640 due to padding?
decode_audio takes its input from the fields of a AVPacket encode_audio should produce a AVPacket (various advantages like reallocating a oo small buffer make this a good idea) if we would return a AVPacket then there would be a duration field that could naturally carry this information
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size);
i think the application is or at least should be required to allocate full frames even for the possibly smaller last
Where should this be documented? avcodec_encode_audio()?
yes and close to CODEC_CAP_SMALL_LAST_FRAME [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote:
Hi,
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke.
The rate control is not as intuitive to use as I would like, but it works. libspeex has the option to have the library choose a CBR bitrate based on a quality setting. Providing that option doesn't really fit well into our current system since there is no way to tell if the user is specifying CBR quality or VBR quality. So instead it just uses bitrate for CBR and quality for VBR like our other audio encoders. The default bitrate of 64kbps is higher than the maximum Speex bitrate, so at least it will be good quality by default. [...] +static av_cold int encode_init(AVCodecContext *avctx) +{ + LibSpeexEncContext *s = avctx->priv_data; + const SpeexMode *mode; + uint8_t *header_data; + int header_size; + int32_t complexity; + + /* channels */ + if (avctx->channels < 1 || avctx->channels > 2) { + av_log(avctx, AV_LOG_ERROR, "Invalid channels (%d). Only stereo and " + "mono are supported\n", avctx->channels); + return -1; + } + + /* sample rate and encoding mode */ + switch (avctx->sample_rate) { + case 8000: mode = &speex_nb_mode; break; + case 16000: mode = &speex_wb_mode; break; + case 32000: mode = &speex_uwb_mode; break; + default: + av_log(avctx, AV_LOG_ERROR, "Sample rate of %d Hz is not supported. " + "Resample to 8, 16, or 32 kHz.\n", avctx->sample_rate); + return -1; + } + + /* initialize libspeex */ + s->enc_state = speex_encoder_init(mode); + if (!s->enc_state) { + av_log(avctx, AV_LOG_ERROR, "Error initializing libspeex\n"); + return -1; + } + speex_init_header(&s->header, avctx->sample_rate, avctx->channels, mode); + + /* rate control method and parameters */ + if (avctx->flags & CODEC_FLAG_QSCALE) { + /* VBR */ + s->header.vbr = 1; + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR, &s->header.vbr); + s->vbr_quality = av_clipf(avctx->global_quality / (float)FF_QP2LAMBDA, + 0.0f, 10.0f); + speex_encoder_ctl(s->enc_state, SPEEX_SET_VBR_QUALITY, &s->vbr_quality); + avctx->bit_rate = 0; + } else { + /* CBR */ + s->header.bitrate = avctx->bit_rate; + speex_encoder_ctl(s->enc_state, SPEEX_SET_BITRATE, &s->header.bitrate); + speex_encoder_ctl(s->enc_state, SPEEX_GET_BITRATE, &s->header.bitrate); + /* stereo side information adds about 800 bps to the base bitrate */ + avctx->bit_rate = s->header.bitrate + (avctx->channels == 2 ? 800 : 0); avctx->bit_rate is set by the user and not the encoder The reason for this is for feedback to the user. libspeex uses the closest supported bitrate for the selected mode that is less than or equal to the requested bitrate. I guess this could be taken out and just let libspeex do whatever without telling the user except maybe in a debug printout. The bitrate is a nominal value anyway. The exact bitrate also depends on the number of frames per packet because frames are not byte aligned, packets are.
hmm, ive no good comment/idea ATM :(
With some added tables and calculations I could come up with the closest true bitrate based on the mode, channels, and frames per packet. Then print a warning message that includes what the real bitrate will be if it is not an exact match to what the user specifies.
[...]
[...]
+static int encode_frame(AVCodecContext *avctx, uint8_t *frame, int buf_size, + void *data) +{ + LibSpeexEncContext *s = avctx->priv_data; + void *samples = data; + int nframes, i; + + if (!avctx->frame_size) + return 0; + + /* handle last packet, which may have fewer frames-per-packet and/or + fewer samples in the last frame */ + nframes = s->header.frames_per_packet; + if (avctx->frame_size < nframes * s->header.frame_size) { + nframes = (avctx->frame_size + s->header.frame_size - 1) / + s->header.frame_size; + if (avctx->frame_size != s->header.frame_size * nframes) { + /* allocate new buffer to pad last frame */ + int new_samples_size; + avctx->frame_size = nframes * s->header.frame_size; iam not sure if this violates the API but at least i would say it is unexpected by the application Hmmm. Yeah, if it doesn't violate API, it is at least not documented. Is there another way to report the correct duration of the output frame if the user gives, for example, 500 samples and the output frame represents 640 due to padding?
decode_audio takes its input from the fields of a AVPacket encode_audio should produce a AVPacket (various advantages like reallocating a oo small buffer make this a good idea) if we would return a AVPacket then there would be a duration field that could naturally carry this information
:) I was both hoping and dreading you would say that. I've been pondering this option since we started using AVPackets for decoding. I'll work on it. Thanks, Justin
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size); i think the application is or at least should be required to allocate full
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote: frames even for the possibly smaller last Where should this be documented? avcodec_encode_audio()?
yes and close to CODEC_CAP_SMALL_LAST_FRAME
Should the API require that the rest of the frame be padded with zeros? The encoder cannot modify the data, so if this is not required, the data would have to be copied in the encoder where there remaining samples could be zeroed out... thus making the allocation requirement useless. -Justin
On Wed, Oct 28, 2009 at 07:45:45PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size); i think the application is or at least should be required to allocate full
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote: frames even for the possibly smaller last Where should this be documented? avcodec_encode_audio()?
yes and close to CODEC_CAP_SMALL_LAST_FRAME
Should the API require that the rest of the frame be padded with zeros? The encoder cannot modify the data, so if this is not required, the data would have to be copied in the encoder where there remaining samples could be zeroed out... thus making the allocation requirement useless.
This actually brings up another issue ... is zero padding even required / a good idea? If these samples are never output then other forms of padding might lead to higher quality & lower bitrate of the last frame. The simplest thing to try would be simple reflecting the values that is: instead of ABCDEF000000 ABCDEFEDCBA or ABCDEFFEDCBA [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Michael Niedermayer wrote:
On Wed, Oct 28, 2009 at 07:45:45PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size); i think the application is or at least should be required to allocate full
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote: frames even for the possibly smaller last Where should this be documented? avcodec_encode_audio()? yes and close to CODEC_CAP_SMALL_LAST_FRAME Should the API require that the rest of the frame be padded with zeros? The encoder cannot modify the data, so if this is not required, the data would have to be copied in the encoder where there remaining samples could be zeroed out... thus making the allocation requirement useless.
This actually brings up another issue ... is zero padding even required / a good idea? If these samples are never output then other forms of padding might lead to higher quality & lower bitrate of the last frame. The simplest thing to try would be simple reflecting the values that is: instead of ABCDEF000000 ABCDEFEDCBA or ABCDEFFEDCBA
For 20ms at the end of a stream does it really matter? For Speex encoding, I went with zero padding because that's what speexenc does. If the duration of the last packet is set to the smaller size, yes speexdec will chop off the padded samples. Our libspeex decoder doesn't do that though (I have a patch but I think it's horribly ugly and it probably won't work with seeking). -Justin
On Thu, Oct 29, 2009 at 06:16:31PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Wed, Oct 28, 2009 at 07:45:45PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote: > + new_samples_size = avctx->frame_size * avctx->channels * > + (avctx->sample_fmt == SAMPLE_FMT_FLT ? > + sizeof(float) : sizeof(int16_t)); > + samples = av_mallocz(new_samples_size); > + if (!samples) > + return AVERROR(ENOMEM); > + memcpy(samples, data, new_samples_size); i think the application is or at least should be required to allocate full frames even for the possibly smaller last Where should this be documented? avcodec_encode_audio()? yes and close to CODEC_CAP_SMALL_LAST_FRAME Should the API require that the rest of the frame be padded with zeros? The encoder cannot modify the data, so if this is not required, the data would have to be copied in the encoder where there remaining samples could be zeroed out... thus making the allocation requirement useless.
This actually brings up another issue ... is zero padding even required / a good idea? If these samples are never output then other forms of padding might lead to higher quality & lower bitrate of the last frame. The simplest thing to try would be simple reflecting the values that is: instead of ABCDEF000000 ABCDEFEDCBA or ABCDEFFEDCBA
For 20ms at the end of a stream does it really matter? For Speex
well, probably not, unless you have many short files ... it was just an idea, as we dont zero pad video either and that helps quality/bitrate of the borders ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"
On Wed, Oct 28, 2009 at 07:45:45PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Oct 27, 2009 at 05:54:51PM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
+ new_samples_size = avctx->frame_size * avctx->channels * + (avctx->sample_fmt == SAMPLE_FMT_FLT ? + sizeof(float) : sizeof(int16_t)); + samples = av_mallocz(new_samples_size); + if (!samples) + return AVERROR(ENOMEM); + memcpy(samples, data, new_samples_size); i think the application is or at least should be required to allocate full
On Sun, Oct 25, 2009 at 09:04:45AM -0400, Justin Ruggles wrote: frames even for the possibly smaller last Where should this be documented? avcodec_encode_audio()?
yes and close to CODEC_CAP_SMALL_LAST_FRAME
Should the API require that the rest of the frame be padded with zeros? The encoder cannot modify the data, so if this is not required, the data would have to be copied in the encoder where there remaining samples could be zeroed out... thus making the allocation requirement useless.
its simpler (for applications and encoders) if the zero padding and actually also the allocation of a full frame is done in avcodec_encode_audio() [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In fact, the RIAA has been known to suggest that students drop out of college or go to community college in order to be able to afford settlements. -- The RIAA
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit?:
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch. Regards, -- Nicolas George
On Tue, Jun 22, 2010 at 10:31:23AM +0200, Nicolas George wrote:
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit?:
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch.
waiting for a new revission that considers all comments dunno if there are open questions left [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
Michael Niedermayer wrote:
On Tue, Jun 22, 2010 at 10:31:23AM +0200, Nicolas George wrote:
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit :
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch.
waiting for a new revission that considers all comments dunno if there are open questions left
I have an updated patch ready. Well, 2 patches. It needs an audio_frames_per_packet field added to AVCodecContext. I just need to double-check everything before sending it in. -Justin
On Tue, Sep 07, 2010 at 06:22:58AM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Jun 22, 2010 at 10:31:23AM +0200, Nicolas George wrote:
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit :
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch.
waiting for a new revission that considers all comments dunno if there are open questions left
I have an updated patch ready. Well, 2 patches. It needs an audio_frames_per_packet field added to AVCodecContext.
Are you sure this will work? If it's possible to have a speex stream where this value changes, having it in AVCodecContext will result in the same race condition as the PaletteControl stuff, and thus be just wrong. If it doesn't change it might be nicer to munge it into extradata so it will work find with stream copy into e.g. AVI, nut, ...
Reimar D?ffinger wrote:
On Tue, Sep 07, 2010 at 06:22:58AM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Jun 22, 2010 at 10:31:23AM +0200, Nicolas George wrote:
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit :
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch. waiting for a new revission that considers all comments dunno if there are open questions left I have an updated patch ready. Well, 2 patches. It needs an audio_frames_per_packet field added to AVCodecContext.
Are you sure this will work? If it's possible to have a speex stream where this value changes, having it in AVCodecContext will result in the same race condition as the PaletteControl stuff, and thus be just wrong. If it doesn't change it might be nicer to munge it into extradata so it will work find with stream copy into e.g. AVI, nut, ...
It should not change. It is in the global header, and in-band signals do not allow it to be changed. Even in the final packet, dummy frames are used at the end to make it constant. For stream copy it really isn't necessary. And for Speex it is already in the extradata (except in FLV where there is no Speex header, but that is a different issue). The real need is for encoding. We need to give the user the option to tell the encoder the number of frames per packet to use. -Justin
On Tue, Sep 07, 2010 at 05:21:56PM -0400, Justin Ruggles wrote:
Reimar D?ffinger wrote:
On Tue, Sep 07, 2010 at 06:22:58AM -0400, Justin Ruggles wrote:
Michael Niedermayer wrote:
On Tue, Jun 22, 2010 at 10:31:23AM +0200, Nicolas George wrote:
Le quartidi 4 brumaire, an CCXVIII, Justin Ruggles a ?crit :
This patch combines parts of my previous libspeex encoding patch with parts of the one sent by Art Clarke. <snip>
Excuse-me, but I wonder what has become of this patch. waiting for a new revission that considers all comments dunno if there are open questions left I have an updated patch ready. Well, 2 patches. It needs an audio_frames_per_packet field added to AVCodecContext.
Are you sure this will work? If it's possible to have a speex stream where this value changes, having it in AVCodecContext will result in the same race condition as the PaletteControl stuff, and thus be just wrong. If it doesn't change it might be nicer to munge it into extradata so it will work find with stream copy into e.g. AVI, nut, ...
It should not change. It is in the global header, and in-band signals do not allow it to be changed. Even in the final packet, dummy frames are used at the end to make it constant.
For stream copy it really isn't necessary. And for Speex it is already in the extradata (except in FLV where there is no Speex header, but that is a different issue).
The real need is for encoding. We need to give the user the option to tell the encoder the number of frames per packet to use.
Sorry, I managed to not see the word "encoding" there. My fault...
participants (6)
-
diego＠biurrun.de -
justin.ruggles＠gmail.com -
mans＠mansr.com -
michaelni＠gmx.at -
nicolas.george＠normalesup.org -
Reimar.Doeffinger＠gmx.de