diff mbox

[FFmpeg-devel] avformat/mux: stop delaying writing the header

Message ID 20170907191724.9956-1-jamrial@gmail.com
State Superseded
Headers show

Commit Message

James Almer Sept. 7, 2017, 7:17 p.m. UTC 
There's no need to wait for the first packet of every stream now that
every bitstream filter behaves as intended.

Signed-off-by: James Almer <jamrial@gmail.com>
---
What should we do with the AVFMT_FLAG_AUTO_BSF flag? Do we deprecate
it and force the automatic insertion of muxer-required bitstream
filters now that the generic muxing code will always behave the same,
or keep it to give the user a choice to apply said bitstream filters?
I ask because some filters, like vp9_superframe, are necessary to
avoid creating broken files, so it's not wise to allow the user to
disable it.
It would also go in line with AVCodec.bsfs, which is essentially a
non-optional (for reasons made obvious in fa1749dd34) autobsf at the
codec level instead of container level.

The change to fate-rgb24-mkv is a regression that can be prevented by
committing https://ffmpeg.org/pipermail/ffmpeg-devel/2017-May/211311.html
or a similar solution, maybe using avformat_init_output(), to make sure
ffmpeg.c sets AVCodecParameters->field_order for the output stream before
calling avformat_write_header().

 libavformat/internal.h         |  6 -----
 libavformat/mux.c              | 51 ++++++++---------------------------------
 libavformat/options_table.h    |  2 +-
 libavformat/tests/fifo_muxer.c | 52 ------------------------------------------
 tests/ref/fate/fifo-muxer-tst  |  1 -
 tests/ref/fate/rgb24-mkv       |  4 ++--
 6 files changed, 13 insertions(+), 103 deletions(-)

Comments

Michael Niedermayer Sept. 9, 2017, 3:41 p.m. UTC | #1 
On Thu, Sep 07, 2017 at 04:17:24PM -0300, James Almer wrote:
> There's no need to wait for the first packet of every stream now that
> every bitstream filter behaves as intended.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> What should we do with the AVFMT_FLAG_AUTO_BSF flag? Do we deprecate
> it and force the automatic insertion of muxer-required bitstream
> filters now that the generic muxing code will always behave the same,
> or keep it to give the user a choice to apply said bitstream filters?
> I ask because some filters, like vp9_superframe, are necessary to
> avoid creating broken files, so it's not wise to allow the user to
> disable it.
> It would also go in line with AVCodec.bsfs, which is essentially a
> non-optional (for reasons made obvious in fa1749dd34) autobsf at the
> codec level instead of container level.
> 

> The change to fate-rgb24-mkv is a regression that can be prevented by
> committing https://ffmpeg.org/pipermail/ffmpeg-devel/2017-May/211311.html
> or a similar solution, maybe using avformat_init_output(), to make sure
> ffmpeg.c sets AVCodecParameters->field_order for the output stream before
> calling avformat_write_header().

i do see differences in other mkv output, i assume these are also from
field_order

one example
./ffmpeg -copyts -ss 11 -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 moon.mkv

[...]
James Almer Sept. 9, 2017, 4:17 p.m. UTC | #2 
On 9/9/2017 12:41 PM, Michael Niedermayer wrote:
> On Thu, Sep 07, 2017 at 04:17:24PM -0300, James Almer wrote:
>> There's no need to wait for the first packet of every stream now that
>> every bitstream filter behaves as intended.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> What should we do with the AVFMT_FLAG_AUTO_BSF flag? Do we deprecate
>> it and force the automatic insertion of muxer-required bitstream
>> filters now that the generic muxing code will always behave the same,
>> or keep it to give the user a choice to apply said bitstream filters?
>> I ask because some filters, like vp9_superframe, are necessary to
>> avoid creating broken files, so it's not wise to allow the user to
>> disable it.
>> It would also go in line with AVCodec.bsfs, which is essentially a
>> non-optional (for reasons made obvious in fa1749dd34) autobsf at the
>> codec level instead of container level.
>>
> 
>> The change to fate-rgb24-mkv is a regression that can be prevented by
>> committing https://ffmpeg.org/pipermail/ffmpeg-devel/2017-May/211311.html
>> or a similar solution, maybe using avformat_init_output(), to make sure
>> ffmpeg.c sets AVCodecParameters->field_order for the output stream before
>> calling avformat_write_header().
> 
> i do see differences in other mkv output, i assume these are also from
> field_order
> 
> one example
> ./ffmpeg -copyts -ss 11 -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 moon.mkv

Yes, as i said, the solution for this could be making ffmpeg.c use
avformat_init_output() and only call avformat_write_header() after the
output codecpar is "complete".
That being said, ffmpeg.c changing output codecpar->field_order based on
some heuristics like it's currently doing is weird. Or at least wrong in
its current form (There are different patches to change this, and even
suggestions to reimplement the field_order enum values in other threads).
See https://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/215669.html
and https://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/215698.html

I have a set of about 50 patches implementing AVOutputFormat.init() on
most if not all of the remaining muxers for this purpose. I can either
send them to the ml or push them to some repo, to avoid the email spam.
However you prefer.
diff mbox

Patch

diff --git a/libavformat/internal.h b/libavformat/internal.h
index d136c79bdd..d46340029b 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -121,12 +121,6 @@  struct AVFormatInternal {
     int avoid_negative_ts_use_pts;
 
     /**
-     * Whether or not a header has already been written
-     */
-    int header_written;
-    int write_header_ret;
-
-    /**
      * Timestamp of the end of the shortest stream.
      */
     int64_t shortest_end;
diff --git a/libavformat/mux.c b/libavformat/mux.c
index 53ad46df42..6a81faf3fb 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -478,25 +478,6 @@  static void flush_if_needed(AVFormatContext *s)
     }
 }
 
-static int write_header_internal(AVFormatContext *s)
-{
-    if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
-        avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_HEADER);
-    if (s->oformat->write_header) {
-        int ret = s->oformat->write_header(s);
-        if (ret >= 0 && s->pb && s->pb->error < 0)
-            ret = s->pb->error;
-        s->internal->write_header_ret = ret;
-        if (ret < 0)
-            return ret;
-        flush_if_needed(s);
-    }
-    s->internal->header_written = 1;
-    if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
-        avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_UNKNOWN);
-    return 0;
-}
-
 int avformat_init_output(AVFormatContext *s, AVDictionary **options)
 {
     int ret = 0;
@@ -535,11 +516,18 @@  int avformat_write_header(AVFormatContext *s, AVDictionary **options)
         if ((ret = avformat_init_output(s, options)) < 0)
             return ret;
 
-    if (!(s->oformat->check_bitstream && s->flags & AVFMT_FLAG_AUTO_BSF)) {
-        ret = write_header_internal(s);
+    if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
+        avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_HEADER);
+    if (s->oformat->write_header) {
+        int ret = s->oformat->write_header(s);
+        if (ret >= 0 && s->pb && s->pb->error < 0)
+            ret = s->pb->error;
         if (ret < 0)
             goto fail;
+        flush_if_needed(s);
     }
+    if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
+        avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_UNKNOWN);
 
     if (!s->internal->streams_initialized) {
         if ((ret = init_pts(s)) < 0)
@@ -765,12 +753,6 @@  FF_DISABLE_DEPRECATION_WARNINGS
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
-    if (!s->internal->header_written) {
-        ret = s->internal->write_header_ret ? s->internal->write_header_ret : write_header_internal(s);
-        if (ret < 0)
-            goto fail;
-    }
-
     if ((pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) {
         AVFrame *frame = (AVFrame *)pkt->data;
         av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
@@ -786,7 +768,6 @@  FF_ENABLE_DEPRECATION_WARNINGS
             ret = s->pb->error;
     }
 
-fail:
 #if FF_API_LAVF_MERGE_SD
 FF_DISABLE_DEPRECATION_WARNINGS
     if (did_split)
@@ -934,11 +915,6 @@  int av_write_frame(AVFormatContext *s, AVPacket *pkt)
 
     if (!pkt) {
         if (s->oformat->flags & AVFMT_ALLOW_FLUSH) {
-            if (!s->internal->header_written) {
-                ret = s->internal->write_header_ret ? s->internal->write_header_ret : write_header_internal(s);
-                if (ret < 0)
-                    return ret;
-            }
             ret = s->oformat->write_packet(s, NULL);
             flush_if_needed(s);
             if (ret >= 0 && s->pb && s->pb->error < 0)
@@ -1322,14 +1298,8 @@  int av_write_trailer(AVFormatContext *s)
             goto fail;
     }
 
-    if (!s->internal->header_written) {
-        ret = s->internal->write_header_ret ? s->internal->write_header_ret : write_header_internal(s);
-        if (ret < 0)
-            goto fail;
-    }
-
 fail:
-    if (s->internal->header_written && s->oformat->write_trailer) {
+    if (s->oformat->write_trailer) {
         if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
             avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_TRAILER);
         if (ret >= 0) {
@@ -1342,7 +1312,6 @@  fail:
     if (s->oformat->deinit)
         s->oformat->deinit(s);
 
-    s->internal->header_written =
     s->internal->initialized =
     s->internal->streams_initialized = 0;
 
diff --git a/libavformat/options_table.h b/libavformat/options_table.h
index 118086df66..c62b7d71d6 100644
--- a/libavformat/options_table.h
+++ b/libavformat/options_table.h
@@ -57,7 +57,7 @@  static const AVOption avformat_options[] = {
 {"seek2any", "allow seeking to non-keyframes on demuxer level when supported", OFFSET(seek2any), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, D},
 {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" },
 {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" },
-{"autobsf", "add needed bsfs automatically (delays header until each stream's first packet is written)", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" },
+{"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" },
 {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D},
 {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D},
 {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D},
diff --git a/libavformat/tests/fifo_muxer.c b/libavformat/tests/fifo_muxer.c
index e20bd6e7b2..4b63df085e 100644
--- a/libavformat/tests/fifo_muxer.c
+++ b/libavformat/tests/fifo_muxer.c
@@ -238,54 +238,6 @@  fail:
     return ret;
 }
 
-static int fifo_write_header_err_tst(AVFormatContext *oc, AVDictionary **opts,
-                                     const FailingMuxerPacketData *pkt_data)
-{
-    int ret = 0, i;
-    AVPacket pkt;
-
-    av_init_packet(&pkt);
-
-    ret = avformat_write_header(oc, opts);
-    if (ret) {
-        fprintf(stderr, "Unexpected write_header failure: %s\n",
-                av_err2str(ret));
-        goto fail;
-    }
-
-    for (i = 0; i < MAX_TST_PACKETS; i++ ) {
-        ret = prepare_packet(&pkt, pkt_data, i);
-        if (ret < 0) {
-            fprintf(stderr, "Failed to prepare test packet: %s\n",
-                    av_err2str(ret));
-            goto write_trailer_and_fail;
-        }
-        ret = av_write_frame(oc, &pkt);
-        av_packet_unref(&pkt);
-        if (ret < 0) {
-            break;
-        }
-    }
-
-    if (!ret) {
-        fprintf(stderr, "write_packet not failed when supposed to.\n");
-        goto fail;
-    } else if (ret != -1) {
-        fprintf(stderr, "Unexpected write_packet error: %s\n", av_err2str(ret));
-        goto fail;
-    }
-
-    ret = av_write_trailer(oc);
-    if (ret < 0)
-        fprintf(stderr, "Unexpected write_trailer error: %s\n", av_err2str(ret));
-
-    return ret;
-write_trailer_and_fail:
-    av_write_trailer(oc);
-fail:
-    return ret;
-}
-
 static int fifo_overflow_drop_test(AVFormatContext *oc, AVDictionary **opts,
                                    const FailingMuxerPacketData *data)
 {
@@ -403,10 +355,6 @@  const TestCase tests[] = {
          * exactly what was on input */
         {fifo_basic_test, "nonfail test", NULL,1, 0, 0, {0, 0, 0}},
 
-        /* Test that we receive delayed write_header error from one of the write_packet
-         * calls. */
-        {fifo_write_header_err_tst, "write header error test", NULL, 0, -1, 0, {0, 0, 0}},
-
         /* Each write_packet will fail 3 times before operation is successful. If recovery
          * Since recovery is on, fifo muxer should not return any errors. */
         {fifo_basic_test, "recovery test", "attempt_recovery=1:recovery_wait_time=0",
diff --git a/tests/ref/fate/fifo-muxer-tst b/tests/ref/fate/fifo-muxer-tst
index ca7e294860..e1139ee0a8 100644
--- a/tests/ref/fate/fifo-muxer-tst
+++ b/tests/ref/fate/fifo-muxer-tst
@@ -2,7 +2,6 @@  flush count: 1
 pts seen nr: 15
 pts seen: 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14
 nonfail test: ok
-write header error test: ok
 recovery test: ok
 flush count: 1
 pts seen nr: 15
diff --git a/tests/ref/fate/rgb24-mkv b/tests/ref/fate/rgb24-mkv
index 4c357accaf..439b2bc5e9 100644
--- a/tests/ref/fate/rgb24-mkv
+++ b/tests/ref/fate/rgb24-mkv
@@ -1,5 +1,5 @@ 
-55270be3b5d393d770a1dfcb19b68271 *tests/data/fate/rgb24-mkv.matroska
-58345 tests/data/fate/rgb24-mkv.matroska
+09ee413b2d92a6be5e3b18e9e20a1f74 *tests/data/fate/rgb24-mkv.matroska
+58342 tests/data/fate/rgb24-mkv.matroska
 #tb 0: 1/10
 #media_type 0: video
 #codec_id 0: rawvideo