| Message ID | 20171126205104.1064-2-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 26.11.2017 21:51, James Almer wrote: > Every bitstream filter behaves as intended now, so there's no need to > wait for the first packet of every stream. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/avformat.h | 2 +- > libavformat/internal.h | 6 ----- > libavformat/mux.c | 52 ++++++++---------------------------------- > libavformat/options_table.h | 2 +- > libavformat/tests/fifo_muxer.c | 52 ------------------------------------------ > tests/ref/fate/fifo-muxer-tst | 1 - > 6 files changed, 12 insertions(+), 103 deletions(-) > > [...] I'm just curious: Will this fix ticket #6375? Regards, Tobias
On 11/27/2017 6:00 AM, Tobias Rapp wrote: > On 26.11.2017 21:51, James Almer wrote: >> Every bitstream filter behaves as intended now, so there's no need to >> wait for the first packet of every stream. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/avformat.h | 2 +- >> libavformat/internal.h | 6 ----- >> libavformat/mux.c | 52 >> ++++++++---------------------------------- >> libavformat/options_table.h | 2 +- >> libavformat/tests/fifo_muxer.c | 52 >> ------------------------------------------ >> tests/ref/fate/fifo-muxer-tst | 1 - >> 6 files changed, 12 insertions(+), 103 deletions(-) >> >> [...] > > I'm just curious: Will this fix ticket #6375? > > Regards, > Tobias Unlikely. That's from ffmpeg.c buffering logic which I'm not changing here.
On Sun, Nov 26, 2017 at 05:51:03PM -0300, James Almer wrote: > Every bitstream filter behaves as intended now, so there's no need to > wait for the first packet of every stream. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/avformat.h | 2 +- > libavformat/internal.h | 6 ----- > libavformat/mux.c | 52 ++++++++---------------------------------- > libavformat/options_table.h | 2 +- > libavformat/tests/fifo_muxer.c | 52 ------------------------------------------ > tests/ref/fate/fifo-muxer-tst | 1 - > 6 files changed, 12 insertions(+), 103 deletions(-) Is this intended to change any files ? this changes slightly: ./ffmpeg -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 file.mkv not saying this is a issue in the patch or anything (i am in favor of the simplification in fact), just that i saw this change [...]
On 11/27/2017 8:18 PM, Michael Niedermayer wrote: > On Sun, Nov 26, 2017 at 05:51:03PM -0300, James Almer wrote: >> Every bitstream filter behaves as intended now, so there's no need to >> wait for the first packet of every stream. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/avformat.h | 2 +- >> libavformat/internal.h | 6 ----- >> libavformat/mux.c | 52 ++++++++---------------------------------- >> libavformat/options_table.h | 2 +- >> libavformat/tests/fifo_muxer.c | 52 ------------------------------------------ >> tests/ref/fate/fifo-muxer-tst | 1 - >> 6 files changed, 12 insertions(+), 103 deletions(-) > > Is this intended to change any files ? > > this changes slightly: > ./ffmpeg -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 file.mkv > > not saying this is a issue in the patch or anything (i am in favor of > the simplification in fact), just that i saw this change I can't access that file, but just in case it's related, this patch is meant to be applied after the first. Otherwise, as the first patch states, stream parameters that might be changed by ffmpeg.c while processing an input frame (specifically field_order) will not be taken into account during write_header. You might remember i have sent this patch alone before, and how it affected matroska files because of differing field_order values, including fate-rgb24-mkv. For that matter, ffmpeg.c changing any output codecpar values while reading an input frame (that is, long after the output file and streams have been initialized) is a very hacky thing to do. This is something I'd expect avformat_find_stream_info() to do instead, since it decodes some frames to get all the info it needs, and not the API user (ffmpeg.c).
2017-11-28 0:52 GMT+01:00 James Almer <jamrial@gmail.com>: > On 11/27/2017 8:18 PM, Michael Niedermayer wrote: >> this changes slightly: >> ./ffmpeg -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 file.mkv >> >> not saying this is a issue in the patch or anything (i am in favor of >> the simplification in fact), just that i saw this change > > I can't access that file http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2263/ Will remove it again. Carl Eugen
On 11/27/2017 9:00 PM, Carl Eugen Hoyos wrote: > 2017-11-28 0:52 GMT+01:00 James Almer <jamrial@gmail.com>: >> On 11/27/2017 8:18 PM, Michael Niedermayer wrote: > >>> this changes slightly: >>> ./ffmpeg -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 file.mkv >>> >>> not saying this is a issue in the patch or anything (i am in favor of >>> the simplification in fact), just that i saw this change >> >> I can't access that file > > http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2263/ > Will remove it again. Thanks.
On 11/27/2017 8:52 PM, James Almer wrote: > On 11/27/2017 8:18 PM, Michael Niedermayer wrote: >> On Sun, Nov 26, 2017 at 05:51:03PM -0300, James Almer wrote: >>> Every bitstream filter behaves as intended now, so there's no need to >>> wait for the first packet of every stream. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/avformat.h | 2 +- >>> libavformat/internal.h | 6 ----- >>> libavformat/mux.c | 52 ++++++++---------------------------------- >>> libavformat/options_table.h | 2 +- >>> libavformat/tests/fifo_muxer.c | 52 ------------------------------------------ >>> tests/ref/fate/fifo-muxer-tst | 1 - >>> 6 files changed, 12 insertions(+), 103 deletions(-) >> >> Is this intended to change any files ? >> >> this changes slightly: >> ./ffmpeg -i ~/tickets/2263/2263-slow-ss.mkv -vframes 3 file.mkv >> >> not saying this is a issue in the patch or anything (i am in favor of >> the simplification in fact), just that i saw this change > > I can't access that file, but just in case it's related, this patch is > meant to be applied after the first. Otherwise, as the first patch > states, stream parameters that might be changed by ffmpeg.c while > processing an input frame (specifically field_order) will not be taken > into account during write_header. > You might remember i have sent this patch alone before, and how it > affected matroska files because of differing field_order values, > including fate-rgb24-mkv. > > For that matter, ffmpeg.c changing any output codecpar values while > reading an input frame (that is, long after the output file and streams > have been initialized) is a very hacky thing to do. This is something > I'd expect avformat_find_stream_info() to do instead, since it decodes > some frames to get all the info it needs, and not the API user (ffmpeg.c). Just tested it, and it was indeed field_order in the output codecpar. And it unfortunately happens with both patches applied. Looks like the packet that gets processed by ffmpeg.c to fill field_order is not necessarily the first to show up, and the only way to effectively write the header after it is with the delayed header code as it's implemented in av_interleaved_write_frame. So yeah, patch 1 can be applied or dropped as preferred (it after all does at least cover some cases, like fate-rgb24-mkv), but this one should still be applied regardless of the above.
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 322210fae0..4f2798a871 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -1449,7 +1449,7 @@ typedef struct AVFormatContext { #endif #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. -#define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Wait for packet data before writing a header, and add bitstream filters as requested by the muxer +#define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer /** * Maximum size of the data read from input for determining diff --git a/libavformat/internal.h b/libavformat/internal.h index fcd47840a5..36a57214ce 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -120,12 +120,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. */ diff --git a/libavformat/mux.c b/libavformat/mux.c index b1244c67f3..ebb9102f11 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -458,25 +458,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; @@ -515,11 +496,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) @@ -739,12 +727,6 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) } } - 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); @@ -760,8 +742,6 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) ret = s->pb->error; } -fail: - if (ret < 0) { pkt->pts = pts_backup; pkt->dts = dts_backup; @@ -894,11 +874,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) @@ -1282,14 +1257,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) { @@ -1302,7 +1271,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 b60d031f67..b8fa47c6fd 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
Every bitstream filter behaves as intended now, so there's no need to wait for the first packet of every stream. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/avformat.h | 2 +- libavformat/internal.h | 6 ----- libavformat/mux.c | 52 ++++++++---------------------------------- libavformat/options_table.h | 2 +- libavformat/tests/fifo_muxer.c | 52 ------------------------------------------ tests/ref/fate/fifo-muxer-tst | 1 - 6 files changed, 12 insertions(+), 103 deletions(-)