[PATCH] avcodec: Allow user applications to provide non padded input buffers
This can be extended easily to skip the buffer growing for codecs which do not need it. Signed-off-by: Michael Niedermayer <michaelni@gmx.at> --- libavcodec/avcodec.h | 8 +++++++- libavcodec/utils.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99467bb..ec95194 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame, * Some decoders may support multiple frames in a single AVPacket, such * decoders would then just decode the first frame. * - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than + * @warning If you use non AVBuffer based AVPackets, then the input buffer must + * be FF_INPUT_BUFFER_PADDING_SIZE larger than * the actual read bytes because some optimized bitstream readers read 32 or 64 * bits at once and could read over the end. + * If you do use AVBuffer based AVPackets and the allocated space as indicated + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding then + * many decoders will reallocate the buffer. + * Most AVPacket as created by the FFmpeg APIs will already include the needed + * padding. * * @warning The end of the input buffer buf should be set to 0 to ensure that * no overreading happens for damaged MPEG streams. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1ec5cae..eaa0431 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2332,7 +2332,8 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi AVCodecInternal *avci = avctx->internal; int ret; // copy to ensure we do not change avpkt - AVPacket tmp = *avpkt; + AVPacket tmp; + int did_split = 0; if (!avctx->codec) return AVERROR(EINVAL); @@ -2347,8 +2348,15 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi av_frame_unref(picture); + if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE) { + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE)) < 0) + goto fail; + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE; + } + tmp = *avpkt; + if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) { - int did_split = av_packet_split_side_data(&tmp); + did_split = av_packet_split_side_data(&tmp); ret = apply_param_change(avctx, &tmp); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n"); -- 1.7.9.5
On Wed, 14 Jan 2015 17:23:08 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
This can be extended easily to skip the buffer growing for codecs which do not need it.
Signed-off-by: Michael Niedermayer <michaelni@gmx.at> --- libavcodec/avcodec.h | 8 +++++++- libavcodec/utils.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99467bb..ec95194 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame, * Some decoders may support multiple frames in a single AVPacket, such * decoders would then just decode the first frame. * - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than + * @warning If you use non AVBuffer based AVPackets, then the input buffer must + * be FF_INPUT_BUFFER_PADDING_SIZE larger than * the actual read bytes because some optimized bitstream readers read 32 or 64 * bits at once and could read over the end. + * If you do use AVBuffer based AVPackets and the allocated space as indicated + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding then + * many decoders will reallocate the buffer. + * Most AVPacket as created by the FFmpeg APIs will already include the needed + * padding. * * @warning The end of the input buffer buf should be set to 0 to ensure that * no overreading happens for damaged MPEG streams. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1ec5cae..eaa0431 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2332,7 +2332,8 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi AVCodecInternal *avci = avctx->internal; int ret; // copy to ensure we do not change avpkt - AVPacket tmp = *avpkt; + AVPacket tmp; + int did_split = 0;
if (!avctx->codec) return AVERROR(EINVAL); @@ -2347,8 +2348,15 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
av_frame_unref(picture);
+ if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE) { + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE)) < 0) + goto fail; + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE; + } + tmp = *avpkt; +
This assumes that AVBuffer is actually always padded. Is this the case? The user can create custom AVBuffers, with custom allocation and custom free function.
if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) { - int did_split = av_packet_split_side_data(&tmp); + did_split = av_packet_split_side_data(&tmp); ret = apply_param_change(avctx, &tmp); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
What's the point of the did_split change?
On Wed, Jan 14, 2015 at 05:58:04PM +0100, wm4 wrote:
On Wed, 14 Jan 2015 17:23:08 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
This can be extended easily to skip the buffer growing for codecs which do not need it.
Signed-off-by: Michael Niedermayer <michaelni@gmx.at> --- libavcodec/avcodec.h | 8 +++++++- libavcodec/utils.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99467bb..ec95194 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame, * Some decoders may support multiple frames in a single AVPacket, such * decoders would then just decode the first frame. * - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than + * @warning If you use non AVBuffer based AVPackets, then the input buffer must + * be FF_INPUT_BUFFER_PADDING_SIZE larger than * the actual read bytes because some optimized bitstream readers read 32 or 64 * bits at once and could read over the end. + * If you do use AVBuffer based AVPackets and the allocated space as indicated + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding then + * many decoders will reallocate the buffer. + * Most AVPacket as created by the FFmpeg APIs will already include the needed + * padding. * * @warning The end of the input buffer buf should be set to 0 to ensure that * no overreading happens for damaged MPEG streams. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1ec5cae..eaa0431 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2332,7 +2332,8 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi AVCodecInternal *avci = avctx->internal; int ret; // copy to ensure we do not change avpkt - AVPacket tmp = *avpkt; + AVPacket tmp; + int did_split = 0;
if (!avctx->codec) return AVERROR(EINVAL); @@ -2347,8 +2348,15 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
av_frame_unref(picture);
+ if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE) { + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE)) < 0) + goto fail; + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE; + } + tmp = *avpkt; +
This assumes that AVBuffer is actually always padded. Is this the case? The user can create custom AVBuffers, with custom allocation and custom free function.
You mean the user could allocate a sufficicently large buffer but set the AVBuffer size incorrectly, yes this is hypothetically possible and would lead to a unneeded reallocation. What do you suggest ?
if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) { - int did_split = av_packet_split_side_data(&tmp); + did_split = av_packet_split_side_data(&tmp); ret = apply_param_change(avctx, &tmp); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
What's the point of the did_split change?
not to leave it uninitialized on the "goto fail" path, which could be bad [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are best at talking, realize last or never when they are wrong.
On Wed, 14 Jan 2015 18:06:41 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
On Wed, Jan 14, 2015 at 05:58:04PM +0100, wm4 wrote:
On Wed, 14 Jan 2015 17:23:08 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
This can be extended easily to skip the buffer growing for codecs which do not need it.
Signed-off-by: Michael Niedermayer <michaelni@gmx.at> --- libavcodec/avcodec.h | 8 +++++++- libavcodec/utils.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99467bb..ec95194 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame, * Some decoders may support multiple frames in a single AVPacket, such * decoders would then just decode the first frame. * - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than + * @warning If you use non AVBuffer based AVPackets, then the input buffer must + * be FF_INPUT_BUFFER_PADDING_SIZE larger than * the actual read bytes because some optimized bitstream readers read 32 or 64 * bits at once and could read over the end. + * If you do use AVBuffer based AVPackets and the allocated space as indicated + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding then + * many decoders will reallocate the buffer. + * Most AVPacket as created by the FFmpeg APIs will already include the needed + * padding. * * @warning The end of the input buffer buf should be set to 0 to ensure that * no overreading happens for damaged MPEG streams. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1ec5cae..eaa0431 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2332,7 +2332,8 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi AVCodecInternal *avci = avctx->internal; int ret; // copy to ensure we do not change avpkt - AVPacket tmp = *avpkt; + AVPacket tmp; + int did_split = 0;
if (!avctx->codec) return AVERROR(EINVAL); @@ -2347,8 +2348,15 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
av_frame_unref(picture);
+ if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE) { + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE)) < 0) + goto fail; + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE; + } + tmp = *avpkt;
Staring at this again... tmp.size is set, and then the entire tmp variable is overwitten with *avpkt?
+
This assumes that AVBuffer is actually always padded. Is this the case? The user can create custom AVBuffers, with custom allocation and custom free function.
You mean the user could allocate a sufficicently large buffer but set the AVBuffer size incorrectly, yes this is hypothetically possible and would lead to a unneeded reallocation. What do you suggest ?
I overlooked that the AVBuffer uses the padded size, while AVPacket uses the data size. So my comment is invalid, and the code should work. Though I think AVPacket.data doesn't necessarily have to point at AVBuffer start - and maybe not into the AVBuffer at all? Also, do you have any future plans for this? As of now, the only change in behavior is when the user sets up his own AVBuffers.
if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) { - int did_split = av_packet_split_side_data(&tmp); + did_split = av_packet_split_side_data(&tmp); ret = apply_param_change(avctx, &tmp); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
What's the point of the did_split change?
not to leave it uninitialized on the "goto fail" path, which could be bad
OK.
On Wed, Jan 14, 2015 at 07:22:48PM +0100, wm4 wrote:
On Wed, 14 Jan 2015 18:06:41 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
On Wed, Jan 14, 2015 at 05:58:04PM +0100, wm4 wrote:
On Wed, 14 Jan 2015 17:23:08 +0100 Michael Niedermayer <michaelni@gmx.at> wrote:
This can be extended easily to skip the buffer growing for codecs which do not need it.
Signed-off-by: Michael Niedermayer <michaelni@gmx.at> --- libavcodec/avcodec.h | 8 +++++++- libavcodec/utils.c | 12 ++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99467bb..ec95194 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4128,9 +4128,15 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame, * Some decoders may support multiple frames in a single AVPacket, such * decoders would then just decode the first frame. * - * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than + * @warning If you use non AVBuffer based AVPackets, then the input buffer must + * be FF_INPUT_BUFFER_PADDING_SIZE larger than * the actual read bytes because some optimized bitstream readers read 32 or 64 * bits at once and could read over the end. + * If you do use AVBuffer based AVPackets and the allocated space as indicated + * by the AVBuffer does not include FF_INPUT_BUFFER_PADDING_SIZE padding then + * many decoders will reallocate the buffer. + * Most AVPacket as created by the FFmpeg APIs will already include the needed + * padding. * * @warning The end of the input buffer buf should be set to 0 to ensure that * no overreading happens for damaged MPEG streams. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1ec5cae..eaa0431 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2332,7 +2332,8 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi AVCodecInternal *avci = avctx->internal; int ret; // copy to ensure we do not change avpkt - AVPacket tmp = *avpkt; + AVPacket tmp; + int did_split = 0;
if (!avctx->codec) return AVERROR(EINVAL); @@ -2347,8 +2348,15 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
av_frame_unref(picture);
+ if (avpkt->buf && avpkt->size && avpkt->buf->size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE) { + if ((ret = av_grow_packet(avpkt, FF_INPUT_BUFFER_PADDING_SIZE)) < 0) + goto fail; + tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE; + } + tmp = *avpkt;
Staring at this again... tmp.size is set, and then the entire tmp variable is overwitten with *avpkt?
yes, thats broken, i had tested this but simplified it afterwards quite a bit without restesting, ill fix this
+
This assumes that AVBuffer is actually always padded. Is this the case? The user can create custom AVBuffers, with custom allocation and custom free function.
You mean the user could allocate a sufficicently large buffer but set the AVBuffer size incorrectly, yes this is hypothetically possible and would lead to a unneeded reallocation. What do you suggest ?
I overlooked that the AVBuffer uses the padded size, while AVPacket uses the data size. So my comment is invalid, and the code should work.
Though I think AVPacket.data doesn't necessarily have to point at AVBuffer start
hmm, i guess this is true
- and maybe not into the AVBuffer at all?
that would be a very odd thing but if preferred i can just add a field to AVCodecContext in which the user can specify the allocated size of the next packet, this would avoid all ambiguities though it would be more code
Also, do you have any future plans for this? As of now, the only change in behavior is when the user sets up his own AVBuffers.
I had no immedeate plans to do more work on this due to too many other things i wanted to work on but the logical next step would be to mark decoders which do not need the paddding as such and to change decoders which are important like h264 to not need the padding if that turns out to be not too ugly when implemented [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus
participants (2)
-
Michael Niedermayer -
wm4