diff mbox

[FFmpeg-devel,1/8] libavformat/dashenc: use avio_dynbuf instead of packet_write callback

Message ID 20170121143909.29028-2-pegro@friiks.de
State New
Headers show

Commit Message

Peter Große Jan. 21, 2017, 2:39 p.m. UTC 
The dash_write function drops data, if no IOContext is initialized.
This might happen when a subordinate muxer calls avio_flush().
Using a dynamic buffer fixes that.

Signed-off-by: Peter Große <pegro@friiks.de>
---
 libavformat/dashenc.c | 86 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 33 deletions(-)

Comments

Moritz Barsnick Jan. 21, 2017, 9:53 p.m. UTC | #1 
On Sat, Jan 21, 2017 at 15:39:02 +0100, Peter Große wrote:

I do understand that some of this code may be copied or adhering to the
code style near it, but just some style nits anyway:

> +    if(!os->ctx->pb) {
> +        return AVERROR(EINVAL);
> +    }

When adding new code, please stick to "if (" with a space. (Also in
some of your other patches.)

> +        ret = avio_open_dyn_buf(&ctx->pb);
> +        if (ret < 0)
> +            return ret;

and

> +        if ((ret = flush_dynbuf(os, &range_length)) < 0) {
> +            dash_free(s);
> +            return ret;
> +        }

Two different ways of doing the same thing. ;-) (Assignment to ret
embedded in the "if"-clause versus before it.)

Just saying,
Moritz
Peter Große Jan. 22, 2017, 12:33 p.m. UTC | #2 
On Sat, 21 Jan 2017 22:53:56 +0100
Moritz Barsnick <barsnick@gmx.net> wrote:

> When adding new code, please stick to "if (" with a space. (Also in
> some of your other patches.)
>...
> Two different ways of doing the same thing. ;-) (Assignment to ret
> embedded in the "if"-clause versus before it.)

Thanks for pointing that out.

The mess with "ret" exists already in the old code, so I'm not sure, whether I should fix it only in lines I touch or at all occurances.

Another question: the patcheck tool "complains" about "non doxy comments".
What is the correct way of annotating segments of code? Or is it not recommend at all?

Regards,
Peter
Michael Niedermayer Jan. 22, 2017, 10:44 p.m. UTC | #3 
On Sun, Jan 22, 2017 at 01:33:19PM +0100, Peter Große wrote:
> On Sat, 21 Jan 2017 22:53:56 +0100
> Moritz Barsnick <barsnick@gmx.net> wrote:
> 
> > When adding new code, please stick to "if (" with a space. (Also in
> > some of your other patches.)
> >...
> > Two different ways of doing the same thing. ;-) (Assignment to ret
> > embedded in the "if"-clause versus before it.)
> 
> Thanks for pointing that out.
> 
> The mess with "ret" exists already in the old code, so I'm not sure, whether I should fix it only in lines I touch or at all occurances.
> 

> Another question: the patcheck tool "complains" about "non doxy comments".
> What is the correct way of annotating segments of code? Or is it not recommend at all?

Public and private interface should be documented in the header with
doxy compatible comments

static functions are often not documented, but more documentation
certainly is a good thing unless its totally obvious what a function
does and everything surrounding

sections in funtions are generally documented with normal comments
(thats how its done in the codebase not neccessarily the best or
only choice)

but you may want to consider spliting a function into multiple if
it has sections that need documentation

[...]
diff mbox

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 534fa75..d0a2ab5 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -62,11 +62,10 @@  typedef struct Segment {
 typedef struct OutputStream {
     AVFormatContext *ctx;
     int ctx_inited;
-    uint8_t iobuf[32768];
     AVIOContext *out;
     int packets_written;
     char initfile[1024];
-    int64_t init_start_pos;
+    int64_t init_start_pos, pos;
     int init_range_length;
     int nb_segments, segments_size, segment_index;
     Segment **segments;
@@ -100,14 +99,6 @@  typedef struct DASHContext {
     int ambiguous_frame_rate;
 } DASHContext;
 
-static int dash_write(void *opaque, uint8_t *buf, int buf_size)
-{
-    OutputStream *os = opaque;
-    if (os->out)
-        avio_write(os->out, buf, buf_size);
-    return buf_size;
-}
-
 // RFC 6381
 static void set_codec_str(AVFormatContext *s, AVCodecParameters *par,
                           char *str, int size)
@@ -174,6 +165,29 @@  static void set_codec_str(AVFormatContext *s, AVCodecParameters *par,
     }
 }
 
+static int flush_dynbuf(OutputStream *os, int *range_length)
+{
+    uint8_t *buffer;
+    int ret;
+
+    if(!os->ctx->pb) {
+        return AVERROR(EINVAL);
+    }
+
+    // flush everything
+    av_write_frame(os->ctx, NULL);
+    avio_flush(os->ctx->pb);
+
+    // write to output
+    *range_length = avio_close_dyn_buf(os->ctx->pb, &buffer);
+    avio_write(os->out, buffer, *range_length);
+    av_free(buffer);
+
+    // re-open buffer
+    ret = avio_open_dyn_buf(&os->ctx->pb);
+    return ret;
+}
+
 static void dash_free(AVFormatContext *s)
 {
     DASHContext *c = s->priv_data;
@@ -185,7 +199,7 @@  static void dash_free(AVFormatContext *s)
         if (os->ctx && os->ctx_inited)
             av_write_trailer(os->ctx);
         if (os->ctx && os->ctx->pb)
-            av_free(os->ctx->pb);
+            ffio_free_dyn_buf(&os->ctx->pb);
         ff_format_io_close(s, &os->out);
         if (os->ctx)
             avformat_free_context(os->ctx);
@@ -624,9 +638,10 @@  static int dash_init(AVFormatContext *s)
         ctx->avoid_negative_ts = s->avoid_negative_ts;
         ctx->flags = s->flags;
 
-        ctx->pb = avio_alloc_context(os->iobuf, sizeof(os->iobuf), AVIO_FLAG_WRITE, os, NULL, dash_write, NULL);
-        if (!ctx->pb)
-            return AVERROR(ENOMEM);
+        ret = avio_open_dyn_buf(&ctx->pb);
+        if (ret < 0)
+            return ret;
+        ctx->pb->seekable = 0;
 
         if (c->single_file) {
             if (c->single_file_name)
@@ -646,7 +661,6 @@  static int dash_init(AVFormatContext *s)
         if ((ret = avformat_init_output(ctx, &opts)) < 0)
             return ret;
         os->ctx_inited = 1;
-        avio_flush(ctx->pb);
         av_dict_free(&opts);
 
         av_log(s, AV_LOG_VERBOSE, "Representation %d init segment will be written to: %s\n", i, filename);
@@ -688,13 +702,24 @@  static int dash_init(AVFormatContext *s)
 static int dash_write_header(AVFormatContext *s)
 {
     DASHContext *c = s->priv_data;
-    int i, ret;
+    int i, ret, range_length;
     for (i = 0; i < s->nb_streams; i++) {
         OutputStream *os = &c->streams[i];
         if ((ret = avformat_write_header(os->ctx, NULL)) < 0) {
             dash_free(s);
             return ret;
         }
+
+        if ((ret = flush_dynbuf(os, &range_length)) < 0) {
+            dash_free(s);
+            return ret;
+        }
+
+        if (!c->single_file) {
+            ff_format_io_close(s, &os->out);
+        }
+
+        os->pos = os->init_range_length = range_length;
     }
     ret = write_manifest(s, 0);
     if (!ret)
@@ -803,7 +828,6 @@  static int dash_flush(AVFormatContext *s, int final, int stream)
     for (i = 0; i < s->nb_streams; i++) {
         OutputStream *os = &c->streams[i];
         char filename[1024] = "", full_path[1024], temp_path[1024];
-        int64_t start_pos;
         int range_length, index_length = 0;
 
         if (!os->packets_written)
@@ -821,15 +845,6 @@  static int dash_flush(AVFormatContext *s, int final, int stream)
                 continue;
         }
 
-        if (!os->init_range_length) {
-            av_write_frame(os->ctx, NULL);
-            os->init_range_length = avio_tell(os->ctx->pb);
-            if (!c->single_file)
-                ff_format_io_close(s, &os->out);
-        }
-
-        start_pos = avio_tell(os->ctx->pb);
-
         if (!c->single_file) {
             dash_fill_tmpl_params(filename, sizeof(filename), c->media_seg_name, i, os->segment_index, os->bit_rate, os->start_pts);
             snprintf(full_path, sizeof(full_path), "%s%s", c->dirname, filename);
@@ -837,26 +852,31 @@  static int dash_flush(AVFormatContext *s, int final, int stream)
             ret = s->io_open(s, &os->out, temp_path, AVIO_FLAG_WRITE, NULL);
             if (ret < 0)
                 break;
-            write_styp(os->ctx->pb);
         } else {
             snprintf(full_path, sizeof(full_path), "%s%s", c->dirname, os->initfile);
         }
 
-        av_write_frame(os->ctx, NULL);
-        avio_flush(os->ctx->pb);
-        os->packets_written = 0;
+        ret = flush_dynbuf(os, &range_length);
+        if (ret < 0)
+            break;
 
-        range_length = avio_tell(os->ctx->pb) - start_pos;
         if (c->single_file) {
-            find_index_range(s, full_path, start_pos, &index_length);
+            find_index_range(s, full_path, os->pos, &index_length);
         } else {
             ff_format_io_close(s, &os->out);
             ret = avpriv_io_move(temp_path, full_path);
             if (ret < 0)
                 break;
         }
-        add_segment(os, filename, os->start_pts, os->max_pts - os->start_pts, start_pos, range_length, index_length);
+        add_segment(os, filename, os->start_pts, os->max_pts - os->start_pts, os->pos, range_length, index_length);
         av_log(s, AV_LOG_VERBOSE, "Representation %d media segment %d written to: %s\n", i, os->segment_index, full_path);
+
+        // set new position
+        os->packets_written = 0;
+        os->pos += range_length;
+
+        // write chunk header
+        write_styp(os->ctx->pb);
     }
 
     if (c->window_size || (final && c->remove_at_exit)) {