[FFmpeg-devel] avformat/ttaenc: Don't unnecessarily copy packets

Submitted by Andreas Rheinhardt on Dec. 3, 2019, 11:34 a.m.

Details

Message ID 20191203113453.9701-1-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Dec. 3, 2019, 11:34 a.m.
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(-)

Comments

James Almer Dec. 3, 2019, 12:20 p.m.
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)
>
Andreas Rheinhardt Dec. 3, 2019, 1:07 p.m.
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)
> >

Patch hide | download patch | download mbox

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)