| Message ID | 20191203113453.9701-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On 12/3/2019 8:34 AM, Andreas Rheinhardt wrote: > When the tta muxer writes the actual packet data at the end of the > muxing process, it repeatedly copies the packets contained in the > linked list of stored packets to an AVPacket located on the stack via > ff_packet_list_get(), writes the data and unrefs the packet. > > But when one is willing to traverse and free the linked list oneself, > one can get rid of the copying. Why even add a packet list API if we're just going to reimplement it manually in a module to save the equivalent of one call to av_packet_move_ref() of negligible performance? For that matter, the subject makes it sound you're preventing copying the packet payload rather than the AVPacket struct. If at some point this API is made public, AVPacketList may become opaque. I don't think this micro-optimization is worth the trouble. > This also means that the tta muxer does > not rely on sizeof(AVPacket) any more. This is a good idea, but I'd rather instead alloc the buffer packet at init() and keep the clean loop calling ff_packet_list_get(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/ttaenc.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/libavformat/ttaenc.c b/libavformat/ttaenc.c > index 73c29ae936..84e3937141 100644 > --- a/libavformat/ttaenc.c > +++ b/libavformat/ttaenc.c > @@ -122,13 +122,16 @@ static int tta_write_packet(AVFormatContext *s, AVPacket *pkt) > static void tta_queue_flush(AVFormatContext *s) > { > TTAMuxContext *tta = s->priv_data; > - AVPacket pkt; > - > - while (tta->queue) { > - ff_packet_list_get(&tta->queue, &tta->queue_end, &pkt); > - avio_write(s->pb, pkt.data, pkt.size); > - av_packet_unref(&pkt); > + AVPacketList *entry = tta->queue; > + > + while (entry) { > + AVPacketList *next = entry->next; > + avio_write(s->pb, entry->pkt.data, entry->pkt.size); > + av_packet_unref(&entry->pkt); > + av_free(entry); > + entry = next; > } > + tta->queue = tta->queue_end = NULL; > } > > static int tta_write_trailer(AVFormatContext *s) >
On Tue, Dec 3, 2019 at 1:20 PM James Almer <jamrial@gmail.com> wrote: > On 12/3/2019 8:34 AM, Andreas Rheinhardt wrote: > > When the tta muxer writes the actual packet data at the end of the > > muxing process, it repeatedly copies the packets contained in the > > linked list of stored packets to an AVPacket located on the stack via > > ff_packet_list_get(), writes the data and unrefs the packet. > > > > But when one is willing to traverse and free the linked list oneself, > > one can get rid of the copying. > > Why even add a packet list API if we're just going to reimplement it > manually in a module to save the equivalent of one call to > av_packet_move_ref() of negligible performance? > Ok, patch dropped. > For that matter, the subject makes it sound you're preventing copying > the packet payload rather than the AVPacket struct. > > If at some point this API is made public, AVPacketList may become > opaque. > > And I thought that the only thing that changes will be that next moves to the beginning of AVPacketList so that sizeof(AVPacket) can change without affecting the offset of next. (Removing the dependence on sizeof(AVPacket) (and not the performance improvement) was actually my motivation, as it is with my av_packet_move_ref() patches.) > This also means that the tta muxer does > > not rely on sizeof(AVPacket) any more. > > This is a good idea, but I'd rather instead alloc the buffer packet at > init() and keep the clean loop calling ff_packet_list_get(). > > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > --- > > libavformat/ttaenc.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/libavformat/ttaenc.c b/libavformat/ttaenc.c > > index 73c29ae936..84e3937141 100644 > > --- a/libavformat/ttaenc.c > > +++ b/libavformat/ttaenc.c > > @@ -122,13 +122,16 @@ static int tta_write_packet(AVFormatContext *s, > AVPacket *pkt) > > static void tta_queue_flush(AVFormatContext *s) > > { > > TTAMuxContext *tta = s->priv_data; > > - AVPacket pkt; > > - > > - while (tta->queue) { > > - ff_packet_list_get(&tta->queue, &tta->queue_end, &pkt); > > - avio_write(s->pb, pkt.data, pkt.size); > > - av_packet_unref(&pkt); > > + AVPacketList *entry = tta->queue; > > + > > + while (entry) { > > + AVPacketList *next = entry->next; > > + avio_write(s->pb, entry->pkt.data, entry->pkt.size); > > + av_packet_unref(&entry->pkt); > > + av_free(entry); > > + entry = next; > > } > > + tta->queue = tta->queue_end = NULL; > > } > > > > static int tta_write_trailer(AVFormatContext *s) > >
diff --git a/libavformat/ttaenc.c b/libavformat/ttaenc.c index 73c29ae936..84e3937141 100644 --- a/libavformat/ttaenc.c +++ b/libavformat/ttaenc.c @@ -122,13 +122,16 @@ static int tta_write_packet(AVFormatContext *s, AVPacket *pkt) static void tta_queue_flush(AVFormatContext *s) { TTAMuxContext *tta = s->priv_data; - AVPacket pkt; - - while (tta->queue) { - ff_packet_list_get(&tta->queue, &tta->queue_end, &pkt); - avio_write(s->pb, pkt.data, pkt.size); - av_packet_unref(&pkt); + AVPacketList *entry = tta->queue; + + while (entry) { + AVPacketList *next = entry->next; + avio_write(s->pb, entry->pkt.data, entry->pkt.size); + av_packet_unref(&entry->pkt); + av_free(entry); + entry = next; } + tta->queue = tta->queue_end = NULL; } static int tta_write_trailer(AVFormatContext *s)
When the tta muxer writes the actual packet data at the end of the muxing process, it repeatedly copies the packets contained in the linked list of stored packets to an AVPacket located on the stack via ff_packet_list_get(), writes the data and unrefs the packet. But when one is willing to traverse and free the linked list oneself, one can get rid of the copying. This also means that the tta muxer does not rely on sizeof(AVPacket) any more. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/ttaenc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)