[FFmpeg-devel] [PATCH] avformat/matroskadec: use a linked list to queue packets

[wm4]

On Wed, 21 Feb 2018 00:08:45 -0300
James Almer <jamrial at gmail.com> wrote:

> It's more robust and efficient.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavformat/matroskadec.c | 90 +++++++++++++++++++++++++++--------------------
>  1 file changed, 52 insertions(+), 38 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 2faaf9dfb8..241ee5fed1 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -338,9 +338,8 @@ typedef struct MatroskaDemuxContext {
>      int64_t segment_start;
>  
>      /* the packet queue */
> -    AVPacket **packets;
> -    int num_packets;
> -    AVPacket *prev_pkt;
> +    AVPacketList *queue;
> +    AVPacketList *queue_end;
>  
>      int done;
>  
> @@ -2722,11 +2721,14 @@ fail:
>  static int matroska_deliver_packet(MatroskaDemuxContext *matroska,
>                                     AVPacket *pkt)
>  {
> -    if (matroska->num_packets > 0) {
> +    if (matroska->queue) {
>          MatroskaTrack *tracks = matroska->tracks.elem;
>          MatroskaTrack *track;
> -        memcpy(pkt, matroska->packets[0], sizeof(AVPacket));
> -        av_freep(&matroska->packets[0]);
> +        AVPacketList *pktl = matroska->queue;
> +
> +        av_packet_move_ref(pkt, &pktl->pkt);
> +        matroska->queue = pktl->next;
> +        av_free(pktl);
>          track = &tracks[pkt->stream_index];
>          if (track->has_palette) {
>              uint8_t *pal = av_packet_new_side_data(pkt, AV_PKT_DATA_PALETTE, AVPALETTE_SIZE);
> @@ -2737,41 +2739,45 @@ static int matroska_deliver_packet(MatroskaDemuxContext *matroska,
>              }
>              track->has_palette = 0;
>          }
> -        if (matroska->num_packets > 1) {
> -            void *newpackets;
> -            memmove(&matroska->packets[0], &matroska->packets[1],
> -                    (matroska->num_packets - 1) * sizeof(AVPacket *));
> -            newpackets = av_realloc(matroska->packets,
> -                                    (matroska->num_packets - 1) *
> -                                    sizeof(AVPacket *));
> -            if (newpackets)
> -                matroska->packets = newpackets;
> -        } else {
> -            av_freep(&matroska->packets);
> -            matroska->prev_pkt = NULL;
> -        }
> -        matroska->num_packets--;
> +        if (!matroska->queue)
> +            matroska->queue_end = NULL;
>          return 0;
>      }
>  
>      return -1;
>  }
>  
> +static int matroska_queue_packet(MatroskaDemuxContext *matroska, AVPacket *pkt)
> +{
> +    AVPacketList *pktl = av_malloc(sizeof(*pktl));
> +
> +    if (!pktl)
> +        return AVERROR(ENOMEM);
> +    av_packet_move_ref(&pktl->pkt, pkt);
> +    pktl->next = NULL;
> +
> +    if (matroska->queue_end)
> +        matroska->queue_end->next = pktl;
> +    else
> +        matroska->queue = pktl;
> +    matroska->queue_end = pktl;
> +
> +    return 0;
> +}
> +
>  /*
>   * Free all packets in our internal queue.
>   */
>  static void matroska_clear_queue(MatroskaDemuxContext *matroska)
>  {
> -    matroska->prev_pkt = NULL;
> -    if (matroska->packets) {
> -        int n;
> -        for (n = 0; n < matroska->num_packets; n++) {
> -            av_packet_unref(matroska->packets[n]);
> -            av_freep(&matroska->packets[n]);
> -        }
> -        av_freep(&matroska->packets);
> -        matroska->num_packets = 0;
> +    AVPacketList *pktl;
> +
> +    while (pktl = matroska->queue) {
> +        av_packet_unref(&pktl->pkt);
> +        matroska->queue = pktl->next;
> +        av_free(pktl);
>      }
> +    matroska->queue_end = NULL;
>  }
>  
>  static int matroska_parse_laces(MatroskaDemuxContext *matroska, uint8_t **buf,
> @@ -2953,7 +2959,11 @@ static int matroska_parse_rm_audio(MatroskaDemuxContext *matroska,
>          track->audio.buf_timecode = AV_NOPTS_VALUE;
>          pkt->pos                  = pos;
>          pkt->stream_index         = st->index;
> -        dynarray_add(&matroska->packets, &matroska->num_packets, pkt);
> +        ret = matroska_queue_packet(matroska, pkt);
> +        if (ret < 0) {
> +            av_packet_free(&pkt);
> +            return AVERROR(ENOMEM);
> +        }
>      }
>  
>      return 0;
> @@ -3152,8 +3162,11 @@ static int matroska_parse_webvtt(MatroskaDemuxContext *matroska,
>      pkt->duration = duration;
>      pkt->pos = pos;
>  
> -    dynarray_add(&matroska->packets, &matroska->num_packets, pkt);
> -    matroska->prev_pkt = pkt;
> +    err = matroska_queue_packet(matroska, pkt);
> +    if (err < 0) {
> +        av_packet_free(&pkt);
> +        return AVERROR(ENOMEM);
> +    }
>  
>      return 0;
>  }
> @@ -3268,8 +3281,11 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
> -    dynarray_add(&matroska->packets, &matroska->num_packets, pkt);
> -    matroska->prev_pkt = pkt;
> +    res = matroska_queue_packet(matroska, pkt);
> +    if (res < 0) {
> +        av_packet_free(&pkt);
> +        return AVERROR(ENOMEM);
> +    }
>  
>      return 0;
>  
> @@ -3433,7 +3449,6 @@ static int matroska_parse_cluster_incremental(MatroskaDemuxContext *matroska)
>          memset(&matroska->current_cluster, 0, sizeof(MatroskaCluster));
>          matroska->current_cluster_num_blocks = 0;
>          matroska->current_cluster_pos        = avio_tell(matroska->ctx->pb);
> -        matroska->prev_pkt                   = NULL;
>          /* sizeof the ID which was already read */
>          if (matroska->current_id)
>              matroska->current_cluster_pos -= 4;
> @@ -3486,7 +3501,6 @@ static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
>      if (!matroska->contains_ssa)
>          return matroska_parse_cluster_incremental(matroska);
>      pos = avio_tell(matroska->ctx->pb);
> -    matroska->prev_pkt = NULL;
>      if (matroska->current_id)
>          pos -= 4;  /* sizeof the ID which was already read */
>      res         = ebml_parse(matroska, matroska_clusters, &cluster);
> @@ -3671,10 +3685,10 @@ static int webm_clusters_start_with_keyframe(AVFormatContext *s)
>          matroska->current_id = 0;
>          matroska_clear_queue(matroska);
>          if (matroska_parse_cluster(matroska) < 0 ||
> -            matroska->num_packets <= 0) {
> +            !matroska->queue) {
>              break;
>          }
> -        pkt = matroska->packets[0];
> +        pkt = &matroska->queue->pkt;
>          cluster_pos += cluster_length + 12; // 12 is the offset of the cluster id and length.
>          if (!(pkt->flags & AV_PKT_FLAG_KEY)) {
>              rv = 0;

Do you feel like the change actually makes anything easier? The array
realloc mess could probably be simplified by using one of the realloc
helpers. Also, don't we have some packet list helpers that _might_
avoid having to write another copy of linked list append code?

(Just saying, no string opinions from my side.)