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

Submitted by James Almer on Feb. 21, 2018, 3:08 a.m.

Details

Message ID 20180221030845.6056-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Feb. 21, 2018, 3:08 a.m.
It's more robust and efficient.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroskadec.c | 90 +++++++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 38 deletions(-)

Comments

wm4 Feb. 21, 2018, 7:43 a.m.
On Wed, 21 Feb 2018 00:08:45 -0300
James Almer <jamrial@gmail.com> wrote:

> It's more robust and efficient.
> 
> Signed-off-by: James Almer <jamrial@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.)
James Almer Feb. 21, 2018, 5:31 p.m.
On 2/21/2018 4:43 AM, wm4 wrote:
> On Wed, 21 Feb 2018 00:08:45 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> It's more robust and efficient.
>>
>> Signed-off-by: James Almer <jamrial@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.

We go from a realloc, malloc and free mess (just look at what
dynarray_add expands to), to simply one malloc and one free per packet
queued in a simple linked list.
It's cleaner looking, and also somewhat faster.

> Also, don't we have some packet list helpers that _might_
> avoid having to write another copy of linked list append code?

We don't, afaik. Luca I think wrote one like four years ago, but
couldn't decide on a good namespace and the patchset was then forgotten.

> 
> (Just saying, no string opinions from my side.)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

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;