| Message ID | 20170219133542.4302-1-cus@passwd.hu |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 19 Feb 2017 14:35:42 +0100 Marton Balint <cus@passwd.hu> wrote: > Reallocating a wrapped avframe invalidates internal pointers, such as extended > data. > > FFmpeg has another way of passing AVFrames to muxers, but it seems the API > (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavcodec/utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index f4085bf..184821a 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, > AVFrame *padded_frame = NULL; > int ret; > AVPacket user_pkt = *avpkt; > - int needs_realloc = !user_pkt.data; > + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > > *got_packet_ptr = 0; > > @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, > { > int ret; > AVPacket user_pkt = *avpkt; > - int needs_realloc = !user_pkt.data; > + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > > *got_packet_ptr = 0; > I don't understand this logic in the first place. If nothing was encoded (!ret), and avpkt->data is set (why is it set?), then the AVPacket.buf is realllocated (why?) to the packet size (why???) - how does it make sense?
On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Sun, 19 Feb 2017 14:35:42 +0100 > Marton Balint <cus@passwd.hu> wrote: > >> Reallocating a wrapped avframe invalidates internal pointers, such as extended >> data. >> >> FFmpeg has another way of passing AVFrames to muxers, but it seems the API >> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavcodec/utils.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index f4085bf..184821a 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, >> AVFrame *padded_frame = NULL; >> int ret; >> AVPacket user_pkt = *avpkt; >> - int needs_realloc = !user_pkt.data; >> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >> >> *got_packet_ptr = 0; >> >> @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, >> { >> int ret; >> AVPacket user_pkt = *avpkt; >> - int needs_realloc = !user_pkt.data; >> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >> >> *got_packet_ptr = 0; >> > > I don't understand this logic in the first place. If nothing was > encoded (!ret), and avpkt->data is set (why is it set?), then the > AVPacket.buf is realllocated (why?) to the packet size (why???) - how > does it make sense? ret = 0 indicates successfull encode, ie. not an error code. What result code did you expect? AFAIK the realloc is performed to shrink over-sized pre-allocated packets down and save memory. - Hendrik
On Sun, 19 Feb 2017 14:49:34 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: > > On Sun, 19 Feb 2017 14:35:42 +0100 > > Marton Balint <cus@passwd.hu> wrote: > > > >> Reallocating a wrapped avframe invalidates internal pointers, such as extended > >> data. > >> > >> FFmpeg has another way of passing AVFrames to muxers, but it seems the API > >> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. > >> > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> libavcodec/utils.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c > >> index f4085bf..184821a 100644 > >> --- a/libavcodec/utils.c > >> +++ b/libavcodec/utils.c > >> @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, > >> AVFrame *padded_frame = NULL; > >> int ret; > >> AVPacket user_pkt = *avpkt; > >> - int needs_realloc = !user_pkt.data; > >> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > >> > >> *got_packet_ptr = 0; > >> > >> @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, > >> { > >> int ret; > >> AVPacket user_pkt = *avpkt; > >> - int needs_realloc = !user_pkt.data; > >> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > >> > >> *got_packet_ptr = 0; > >> > > > > I don't understand this logic in the first place. If nothing was > > encoded (!ret), and avpkt->data is set (why is it set?), then the > > AVPacket.buf is realllocated (why?) to the packet size (why???) - how > > does it make sense? > > ret = 0 indicates successfull encode, ie. not an error code. What > result code did you expect? Somehow I expected it to return the number of bytes encoded or something, but of course that's nonsense. (Maybe I was confused that the check was !ret and not ret>=0.) > AFAIK the realloc is performed to shrink over-sized pre-allocated > packets down and save memory. OK, so something fishy. Can't it just skip reallocation if the buffer is already minimally sized (which wrapped_avframe.c already does)?
On Sun, 19 Feb 2017, Hendrik Leppkes wrote: > On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: >> On Sun, 19 Feb 2017 14:35:42 +0100 >> Marton Balint <cus@passwd.hu> wrote: >> >>> Reallocating a wrapped avframe invalidates internal pointers, such as extended >>> data. >>> >>> FFmpeg has another way of passing AVFrames to muxers, but it seems the API >>> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> libavcodec/utils.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >>> index f4085bf..184821a 100644 >>> --- a/libavcodec/utils.c >>> +++ b/libavcodec/utils.c >>> @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, >>> AVFrame *padded_frame = NULL; >>> int ret; >>> AVPacket user_pkt = *avpkt; >>> - int needs_realloc = !user_pkt.data; >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >>> >>> *got_packet_ptr = 0; >>> >>> @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, >>> { >>> int ret; >>> AVPacket user_pkt = *avpkt; >>> - int needs_realloc = !user_pkt.data; >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >>> >>> *got_packet_ptr = 0; >>> >> >> I don't understand this logic in the first place. If nothing was >> encoded (!ret), and avpkt->data is set (why is it set?), then the >> AVPacket.buf is realllocated (why?) to the packet size (why???) - how >> does it make sense? > > ret = 0 indicates successfull encode, ie. not an error code. What > result code did you expect? > AFAIK the realloc is performed to shrink over-sized pre-allocated > packets down and save memory. I thought it is there to always provide padding. Regards, Marton
On Sun, 19 Feb 2017, Hendrik Leppkes wrote: > On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: >> On Sun, 19 Feb 2017 14:35:42 +0100 >> Marton Balint <cus@passwd.hu> wrote: >> >>> Reallocating a wrapped avframe invalidates internal pointers, such as extended >>> data. >>> >>> FFmpeg has another way of passing AVFrames to muxers, but it seems the API >>> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> libavcodec/utils.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >>> index f4085bf..184821a 100644 >>> --- a/libavcodec/utils.c >>> +++ b/libavcodec/utils.c >>> @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, >>> AVFrame *padded_frame = NULL; >>> int ret; >>> AVPacket user_pkt = *avpkt; >>> - int needs_realloc = !user_pkt.data; >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >>> >>> *got_packet_ptr = 0; >>> >>> @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, >>> { >>> int ret; >>> AVPacket user_pkt = *avpkt; >>> - int needs_realloc = !user_pkt.data; >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; >>> >>> *got_packet_ptr = 0; >>> >> >> I don't understand this logic in the first place. If nothing was >> encoded (!ret), and avpkt->data is set (why is it set?), then the >> AVPacket.buf is realllocated (why?) to the packet size (why???) - how >> does it make sense? > > ret = 0 indicates successfull encode, ie. not an error code. What > result code did you expect? > AFAIK the realloc is performed to shrink over-sized pre-allocated > packets down and save memory. > So is it OK to apply the patch? Thanks, Marton
On Mon, 20 Feb 2017 21:11:50 +0100 (CET) Marton Balint <cus@passwd.hu> wrote: > On Sun, 19 Feb 2017, Hendrik Leppkes wrote: > > > On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: > >> On Sun, 19 Feb 2017 14:35:42 +0100 > >> Marton Balint <cus@passwd.hu> wrote: > >> > >>> Reallocating a wrapped avframe invalidates internal pointers, such as extended > >>> data. > >>> > >>> FFmpeg has another way of passing AVFrames to muxers, but it seems the API > >>> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. > >>> > >>> Signed-off-by: Marton Balint <cus@passwd.hu> > >>> --- > >>> libavcodec/utils.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c > >>> index f4085bf..184821a 100644 > >>> --- a/libavcodec/utils.c > >>> +++ b/libavcodec/utils.c > >>> @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, > >>> AVFrame *padded_frame = NULL; > >>> int ret; > >>> AVPacket user_pkt = *avpkt; > >>> - int needs_realloc = !user_pkt.data; > >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > >>> > >>> *got_packet_ptr = 0; > >>> > >>> @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, > >>> { > >>> int ret; > >>> AVPacket user_pkt = *avpkt; > >>> - int needs_realloc = !user_pkt.data; > >>> + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; > >>> > >>> *got_packet_ptr = 0; > >>> > >> > >> I don't understand this logic in the first place. If nothing was > >> encoded (!ret), and avpkt->data is set (why is it set?), then the > >> AVPacket.buf is realllocated (why?) to the packet size (why???) - how > >> does it make sense? > > > > ret = 0 indicates successfull encode, ie. not an error code. What > > result code did you expect? > > AFAIK the realloc is performed to shrink over-sized pre-allocated > > packets down and save memory. > > > > So is it OK to apply the patch? My suggestion doesn't work? (Would probably imply adding the padding in wrapped_avframe.c.)
On Tue, 21 Feb 2017, wm4 wrote: > On Mon, 20 Feb 2017 21:11:50 +0100 (CET) > Marton Balint <cus@passwd.hu> wrote: > >> On Sun, 19 Feb 2017, Hendrik Leppkes wrote: >> >> > On Sun, Feb 19, 2017 at 2:41 PM, wm4 <nfxjfg@googlemail.com> wrote: >> >> On Sun, 19 Feb 2017 14:35:42 +0100 >> >> Marton Balint <cus@passwd.hu> wrote: >> >> >> >>> Reallocating a wrapped avframe invalidates internal pointers, such as extended >> >>> data. >> >>> >> >>> FFmpeg has another way of passing AVFrames to muxers, but it seems the API >> >>> (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. >> >>> >> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >> >>> --- >> >>> libavcodec/utils.c | 4 ++-- >> >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >>> [...] >> >> So is it OK to apply the patch? > > My suggestion doesn't work? (Would probably imply adding the padding in > wrapped_avframe.c.) Yes it does work, if I set the buffer size to sizeof(AVFrame) + AV_INPUT_BUFFER_PADDING_SIZE. av_buffer_realloc already checks the size of the buffer, and avoids the reallocation there is no size change. In wrapped_avframe.c I can either lie about the buffer size in av_create_buffer and set it bigger than the actual allocated memory for the data (AVFrame). Or I can mallocz a buffer with the proper size with padding, and move a frame ref to that buffer. Maybe that is better. I will post an updated patch. Thanks, Marton
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index f4085bf..184821a 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1820,7 +1820,7 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx, AVFrame *padded_frame = NULL; int ret; AVPacket user_pkt = *avpkt; - int needs_realloc = !user_pkt.data; + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; *got_packet_ptr = 0; @@ -1964,7 +1964,7 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx, { int ret; AVPacket user_pkt = *avpkt; - int needs_realloc = !user_pkt.data; + int needs_realloc = !user_pkt.data && avctx->codec_id != AV_CODEC_ID_WRAPPED_AVFRAME; *got_packet_ptr = 0;
Reallocating a wrapped avframe invalidates internal pointers, such as extended data. FFmpeg has another way of passing AVFrames to muxers, but it seems the API (av_write_uncoded_frame) is not implemented in the ffmpeg CLI yet. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavcodec/utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)