| Message ID | 20180221030845.6056-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.)
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 >
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;
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(-)