[FFmpeg-devel] [PATCH] avformat/matroskadec: use a linked list to queue packets
James Almer
jamrial at gmail.com
Wed Feb 21 19:31:33 EET 2018
On 2/21/2018 4:43 AM, wm4 wrote:
> 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.
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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list