[FFmpeg-devel] [PATCH 03/10] avformat/utils: use av_packet_alloc() to allocate packets

[James Almer]

On 2/3/2021 7:36 PM, Michael Niedermayer wrote:
> On Wed, Feb 03, 2021 at 07:25:29PM -0300, James Almer wrote:
>> On 2/3/2021 7:23 PM, Michael Niedermayer wrote:
>>> On Mon, Feb 01, 2021 at 07:44:14PM -0300, James Almer wrote:
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> av_get_packet() and av_read_frame() were allowed to be called with
>>>> uninitialized packets in stack, so to keep supporting that we need to leave the
>>>> calls to av_init_packet() in place during the deprecation period.
>>>>
>>>>    libavformat/internal.h |   1 +
>>>>    libavformat/options.c  |  13 +++--
>>>>    libavformat/utils.c    | 105 +++++++++++++++++++++++------------------
>>>>    3 files changed, 68 insertions(+), 51 deletions(-)
>>>
>>> valgrind dislikes this
>>
>> Did you apply patch 10/10 first, like i mentioned in another reply?
>> I mistakenly sent that patch, which introduces an FF_API define, before the
>> patches that make use of it (This one being the first doing so).
> 
> I probably applied them in order yesterday on their numbers and testing today
> if that was wrong ill test again next time this is posted i guess
> iam a little behind today with things i wanted to do

Just checked and i can reproduce it even with patch 10/10 present.
Turns out applying the avidec patch i already have written to make it 
use allocated AVPackets fixes it, which goes as follows:

> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> index 79000f3e81..0dd447aa7f 100644
> --- a/libavformat/avidec.c
> +++ b/libavformat/avidec.c
> @@ -59,7 +59,7 @@ typedef struct AVIStream {
>                               * the MS dshow demuxer */
> 
>      AVFormatContext *sub_ctx;
> -    AVPacket sub_pkt;
> +    AVPacket *sub_pkt;
>      AVBufferRef *sub_buffer;
> 
>      int64_t seek_pos;
> @@ -1124,6 +1124,9 @@ static int read_gab2_sub(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>          if (strcmp(sub_demuxer->name, "srt") && strcmp(sub_demuxer->name, "ass"))
>              goto error;
> 
> +        if (!(ast->sub_pkt = av_packet_alloc()))
> +            goto error;
> +
>          if (!(ast->sub_ctx = avformat_alloc_context()))
>              goto error;
> 
> @@ -1135,7 +1138,7 @@ static int read_gab2_sub(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>          if (!avformat_open_input(&ast->sub_ctx, "", sub_demuxer, NULL)) {
>              if (ast->sub_ctx->nb_streams != 1)
>                  goto error;
> -            ff_read_packet(ast->sub_ctx, &ast->sub_pkt);
> +            ff_read_packet(ast->sub_ctx, ast->sub_pkt);
>              avcodec_parameters_copy(st->codecpar, ast->sub_ctx->streams[0]->codecpar);
>              time_base = ast->sub_ctx->streams[0]->time_base;
>              avpriv_set_pts_info(st, 64, time_base.num, time_base.den);
> @@ -1146,6 +1149,7 @@ static int read_gab2_sub(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>          return 1;
> 
>  error:
> +        av_packet_free(&ast->sub_pkt);
>          av_freep(&ast->sub_ctx);
>          avio_context_free(&pb);
>      }
> @@ -1166,8 +1170,8 @@ static AVStream *get_subtitle_pkt(AVFormatContext *s, AVStream *next_st,
>      for (i = 0; i < s->nb_streams; i++) {
>          st  = s->streams[i];
>          ast = st->priv_data;
> -        if (st->discard < AVDISCARD_ALL && ast && ast->sub_pkt.data) {
> -            ts = av_rescale_q(ast->sub_pkt.dts, st->time_base, AV_TIME_BASE_Q);
> +        if (st->discard < AVDISCARD_ALL && ast && ast->sub_pkt && ast->sub_pkt->data) {
> +            ts = av_rescale_q(ast->sub_pkt->dts, st->time_base, AV_TIME_BASE_Q);
>              if (ts <= next_ts && ts < ts_min) {
>                  ts_min = ts;
>                  sub_st = st;
> @@ -1177,11 +1181,11 @@ static AVStream *get_subtitle_pkt(AVFormatContext *s, AVStream *next_st,
> 
>      if (sub_st) {
>          ast               = sub_st->priv_data;
> -        *pkt              = ast->sub_pkt;
> +        av_packet_move_ref(pkt, ast->sub_pkt);
>          pkt->stream_index = sub_st->index;
> 
> -        if (ff_read_packet(ast->sub_ctx, &ast->sub_pkt) < 0)
> -            ast->sub_pkt.data = NULL;
> +        if (ff_read_packet(ast->sub_ctx, ast->sub_pkt) < 0)
> +            ast->sub_pkt->data = NULL;
>      }
>      return sub_st;
>  }
> @@ -1800,10 +1804,10 @@ static void seek_subtitle(AVStream *st, AVStream *st2, int64_t timestamp)
>  {
>      AVIStream *ast2 = st2->priv_data;
>      int64_t ts2     = av_rescale_q(timestamp, st->time_base, st2->time_base);
> -    av_packet_unref(&ast2->sub_pkt);
> +    av_packet_unref(ast2->sub_pkt);
>      if (avformat_seek_file(ast2->sub_ctx, 0, INT64_MIN, ts2, ts2, 0) >= 0 ||
>          avformat_seek_file(ast2->sub_ctx, 0, ts2, ts2, INT64_MAX, 0) >= 0)
> -        ff_read_packet(ast2->sub_ctx, &ast2->sub_pkt);
> +        ff_read_packet(ast2->sub_ctx, ast2->sub_pkt);
>  }
> 
>  static int avi_read_seek(AVFormatContext *s, int stream_index,
> @@ -1937,7 +1941,7 @@ static int avi_read_close(AVFormatContext *s)
>                  avformat_close_input(&ast->sub_ctx);
>              }
>              av_buffer_unref(&ast->sub_buffer);
> -            av_packet_unref(&ast->sub_pkt);
> +            av_packet_free(&ast->sub_pkt);
>          }
>      }

Not really happy to see there's such a dependency between generic code 
and a single module (I suspect one cause is the removal of 
av_init_packet() from ff_read_packet(), which requires callers to pass 
an already initialized packet), so I guess i should send all ~50 patches 
that completely get rid of av_init_packet() and all the stack usage i 
could find, instead of just this subset.

If the core deprecation change in patch 10/10 is ok'd, then I'll go 
ahead do it.