[FFmpeg-devel] [PATCH] avformat/spdifenc: handle long TrueHD input_timing gaps
Some TrueHD streams contain frames that have very long gaps in input_timing fields, while output_timing remains constant-rate. These are likely due to encoding discontinuities of some sort as the TrueHD substream terminator marker is observed before the gap. Such frames trigger a sanity check in the current code - however, such gaps are valid. The gaps require us to insert many IEC 61937 bursts worth of MAT padding into the output. To facilitate that, add a mechanism in spdif_write_packet() to allow writing out multiple bursts per AVPacket, and use that in spdif_header_truehd() to write out any full bursts that do not yet contain any actual audio data from the current AVPacket. Modify the sanity check to allow up to 50 MAT frames full of padding. Fixes: incredibles2-truehd-bitstreaming.thd --- I'll apply this soon unless there are comments. libavformat/spdifenc.c | 48 -- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/libavformat/spdifenc.c b/libavformat/spdifenc.c index 0288872fd3..7041ecf2c8 100644 --- a/libavformat/spdifenc.c +++ b/libavformat/spdifenc.c @@ -68,6 +68,7 @@ typedef struct IEC61937Context { int use_preamble; ///< preamble enabled (disabled for exactly pre-padded DTS) int extra_bswap;///< extra bswap for payload (for LE DTS => standard BE DTS) +int more_bursts_needed; ///< more bursts needed for the same AVPacket uint8_t *hd_buf[2]; ///< allocated buffers to concatenate hd audio frames int hd_buf_size;///< size of the hd audio buffer (eac3, dts4) @@ -80,6 +81,7 @@ typedef struct IEC61937Context { uint16_t truehd_prev_time; ///< input_timing from the last frame int truehd_prev_size; ///< previous frame size in bytes, including any MAT codes int truehd_samples_per_frame; ///< samples per frame for padding calculation +int truehd_padding_remaining; ///< amount of padding still needed before data /* AVOptions: */ int dtshd_rate; @@ -450,7 +452,12 @@ static int spdif_header_truehd(AVFormatContext *s, AVPacket *pkt) return AVERROR_INVALIDDATA; input_timing = AV_RB16(pkt->data + 2); -if (ctx->truehd_prev_size) { + +if (ctx->truehd_padding_remaining) { +/* padding was calculated on previous call and some still remains */ +padding_remaining = ctx->truehd_padding_remaining; + +} else if (ctx->truehd_prev_size) { uint16_t delta_samples = input_timing - ctx->truehd_prev_time; /* * One multiple-of-48kHz frame is 1/1200 sec and the IEC 61937 rate @@ -470,8 +477,8 @@ static int spdif_header_truehd(AVFormatContext *s, AVPacket *pkt) delta_samples, delta_bytes); /* sanity check */ -if (padding_remaining < 0 || padding_remaining >= MAT_FRAME_SIZE / 2) { -avpriv_request_sample(s, "Unusual frame timing: %"PRIu16" => %"PRIu16", %d samples/frame", +if (padding_remaining < 0 || padding_remaining >= MAT_PKT_OFFSET * 50) { +avpriv_request_sample(s, "Unusual frame timing (%"PRIu16" => %"PRIu16", %d samples/frame)", ctx->truehd_prev_time, input_timing, ctx->truehd_samples_per_frame); padding_remaining = 0; } @@ -520,6 +527,15 @@ static int spdif_header_truehd(AVFormatContext *s, AVPacket *pkt) /* count the remainder of the code as part of frame size */ if (code_len_remaining) total_frame_size += code_len_remaining; + +if (have_pkt && padding_remaining) { +/* + * We already have a full burst but padding still remains, + * write out the current burst and ask us to be called again + * via ctx->more_bursts_needed to avoid filling our buffers. + */ +break; +} } if (padding_remaining) { @@ -547,9 +563,14 @@ static int spdif_header_truehd(AVFormatContext *s, AVPacket *pkt) ctx->truehd_prev_size = total_frame_size; ctx->truehd_prev_time = input_timing; +ctx->truehd_padding_remaining = padding_remaining; -av_log(s, AV_LOG_TRACE, "TrueHD frame inserted, total size %d, buffer position %d\n", - total_frame_size, ctx->hd_buf_filled); +if (padding_remaining) +av_log(s, AV_LOG_TRACE, "TrueHD frame not yet inserted, %d bytes more padding needed\n", + padding_remaining); +else +av_log(s, AV_LOG_TRACE, "TrueHD frame inserted, total size %d, buffer position %d\n", + total_frame_size, ctx->hd_buf_filled); if (!have_pkt) { ctx->pkt_offset = 0; @@ -560,6 +581,8 @@ static int spdif_header_truehd(AVFormatContext *s, AVPacket *pkt) ctx->pkt_offset = MAT_PKT_OFFSET; ctx->out_bytes = MAT_FRAME_SIZE; ctx->length_code =
[FFmpeg-devel] [PATCH] avformat/mux: allow non-monotonic ts with AVFMT_NOTIMESTAMPS
Allow non-monotonic input timestamps for muxers with no timestamps at all (AVFMT_NOTIMESTAMPS). This is frequently hit when muxing TrueHD with spdifenc as many MKV files use 1ms timestamps while TrueHD frames are shorter than that (1/1200 sec for 48kHz-based and 1/1102.5 sec for 44.1kHz-based rates). --- libavformat/mux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/mux.c b/libavformat/mux.c index 3533932a58..ba6695badb 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -622,7 +622,7 @@ static int compute_muxer_pkt_fields(AVFormatContext *s, AVStream *st, AVPacket * } if (st->cur_dts && st->cur_dts != AV_NOPTS_VALUE && -((!(s->oformat->flags & AVFMT_TS_NONSTRICT) && +((!(s->oformat->flags & (AVFMT_NOTIMESTAMPS | AVFMT_TS_NONSTRICT)) && st->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE && st->codecpar->codec_type != AVMEDIA_TYPE_DATA && st->cur_dts >= pkt->dts) || st->cur_dts > pkt->dts)) { -- 2.24.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] MAINTAINERS: remove myself as hls demuxer maintainer
Anssi Hannula kirjoitti 2018-10-15 00:07: --- Unfortunately I haven't really had the time lately to maintain the HLS demuxer properly and I don't see that changing in the near future. So I intend to apply this removal tomorrow. If anyone is interested in taking over, please do :) I'll continue to maintain the lower-traffic spdif (de)muxers. Applied. MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3dd26e374f..bc2ae13320 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -413,7 +413,6 @@ Muxers/Demuxers: flvenc.c Michael Niedermayer, Steven Liu gxf.c Reimar Doeffinger gxfenc.c Baptiste Coudurier - hls.c Anssi Hannula hlsenc.c Christian Suloway, Steven Liu idcin.c Mike Melanson idroqdec.cMike Melanson -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] MAINTAINERS: remove myself as hls demuxer maintainer
--- Unfortunately I haven't really had the time lately to maintain the HLS demuxer properly and I don't see that changing in the near future. So I intend to apply this removal tomorrow. If anyone is interested in taking over, please do :) I'll continue to maintain the lower-traffic spdif (de)muxers. - Anssi Hannula MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3dd26e374f..bc2ae13320 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -413,7 +413,6 @@ Muxers/Demuxers: flvenc.c Michael Niedermayer, Steven Liu gxf.c Reimar Doeffinger gxfenc.c Baptiste Coudurier - hls.c Anssi Hannula hlsenc.c Christian Suloway, Steven Liu idcin.c Mike Melanson idroqdec.cMike Melanson -- 2.14.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] spdifenc: support ac3 core+eac3 dependent streams
Hi, Hendrik Leppkes kirjoitti 2018-04-03 13:35: Such streams are found on Blu-ray, and identified as EAC3 type in avformat, while the bitstream of the core stream is actually a pure AC3 frame. Adjust the parsing accordingly, since AC3 frames always hold 6 blocks and the numblkscod syntax element is not present. --- libavformat/spdifenc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libavformat/spdifenc.c b/libavformat/spdifenc.c index 3a50aebbef..9514ff8e10 100644 --- a/libavformat/spdifenc.c +++ b/libavformat/spdifenc.c @@ -118,7 +118,8 @@ static int spdif_header_eac3(AVFormatContext *s, AVPacket *pkt) static const uint8_t eac3_repeat[4] = {6, 3, 2, 1}; int repeat = 1; -if ((pkt->data[4] & 0xc0) != 0xc0) /* fscod */ +int bsid = pkt->data[5] >> 3; +if (bsid > 10 && (pkt->data[4] & 0xc0) != 0xc0) /* fscod */ repeat = eac3_repeat[(pkt->data[4] & 0x30) >> 4]; /* numblkscod */ ctx->hd_buf = av_fast_realloc(ctx->hd_buf, >hd_buf_size, ctx->hd_buf_filled + pkt->size); Looks good to me. Thanks, -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 3/5] avformat/hls: add http_persistent option
Aman Gupta kirjoitti 2017-12-17 22:41: On Sun, Dec 17, 2017 at 12:34 PM Anssi Hannula <anssi.hann...@iki.fi> wrote: Hi! Aman Gupta kirjoitti 2017-12-13 02:35: > From: Aman Gupta <a...@tmm1.net> > > This teaches the HLS demuxer to use the HTTP protocols > multiple_requests=1 option, to take advantage of "Connection: > Keep-Alive" when downloading playlists and segments from the HLS > server. > > With the new option, you can avoid TCP connection and TLS negotiation > overhead, which is particularly beneficial when streaming via a > high-latency internet connection. > > Similar to the http_persistent option recently implemented in hlsenc.c Why is this implemented as an option instead of simply being always used by the demuxer? I have no strong feeling on this either way. I've tested the new option against a few different HLS servers and would be comfortable enabling it by default. I do think it's worth keeping as an option in case someone wants to turn it off for whatever reason. OK. The only other demuxer that reuses HTTP connections seems to be rtmphttp, and it does not have an option. I think I'd prefer no option here as well unless a use case is known, but I'm not too strongly against it so I guess it could stay (but default to true). Does anyone else have any thoughts? Also, what happens if the hostname in the URI varies, does it properly open a new connection then? > --- > doc/demuxers.texi | 3 +++ > libavformat/hls.c | 68 > +++ > 2 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/doc/demuxers.texi b/doc/demuxers.texi > index 73dc0feec1..18ff7101ac 100644 > --- a/doc/demuxers.texi > +++ b/doc/demuxers.texi > @@ -316,6 +316,9 @@ segment index to start live streams at (negative > values are from the end). > @item max_reload > Maximum number of times a insufficient list is attempted to be > reloaded. > Default value is 1000. > + > +@item http_persistent > +Use persistent HTTP connections. Applicable only for HTTP streams. > @end table > > @section image2 > diff --git a/libavformat/hls.c b/libavformat/hls.c > index ab6ff187a6..5c1895c180 100644 > --- a/libavformat/hls.c > +++ b/libavformat/hls.c > @@ -26,6 +26,7 @@ > * http://tools.ietf.org/html/draft-pantos-http-live-streaming > */ > > +#include "libavformat/http.h" > #include "libavutil/avstring.h" > #include "libavutil/avassert.h" > #include "libavutil/intreadwrite.h" > @@ -94,6 +95,7 @@ struct playlist { > AVIOContext pb; > uint8_t* read_buffer; > AVIOContext *input; > +int input_read_done; > AVFormatContext *parent; > int index; > AVFormatContext *ctx; > @@ -206,6 +208,8 @@ typedef struct HLSContext { > int strict_std_compliance; > char *allowed_extensions; > int max_reload; > +int http_persistent; > +AVIOContext *playlist_pb; > } HLSContext; > > static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) > @@ -256,6 +260,7 @@ static void free_playlist_list(HLSContext *c) > av_freep(>pb.buffer); > if (pls->input) > ff_format_io_close(c->ctx, >input); > +pls->input_read_done = 0; > if (pls->ctx) { > pls->ctx->pb = NULL; > avformat_close_input(>ctx); > @@ -640,7 +645,24 @@ static int open_url(AVFormatContext *s, > AVIOContext **pb, const char *url, > else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) > return AVERROR_INVALIDDATA; > > -ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); > +if (c->http_persistent && *pb && av_strstart(proto_name, "http", > NULL)) { > +URLContext *uc = ffio_geturlcontext(*pb); > +av_assert0(uc); > +(*pb)->eof_reached = 0; > +ret = ff_http_do_new_request(uc, url); > +if (ret == AVERROR_EXIT) { > +ff_format_io_close(c->ctx, >playlist_pb); > +return ret; > +} else if (ret < 0) { > +av_log(s, AV_LOG_WARNING, > +"keepalive request failed for '%s', retrying with new > connection: %s\n", > +url, av_err2str(ret)); > +ff_format_io_close(c->ctx, pb); > +ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); > +} > +} else { > +ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); > +} > if (ret >= 0) { > // update cookies on http response with setcookies. > char *new_cookies = NULL; > @@ -683,10 +705,27
Re: [FFmpeg-devel] [PATCH v2 5/5] avformat/hls: add http_multiple option
Hi! Aman Gupta kirjoitti 2017-12-13 02:35: From: Aman Gupta <a...@tmm1.net> This improves network throughput of the hls demuxer by avoiding the latency introduced by downloading segments one at a time. The problem is particularly noticable over high-latency network connections: for instance, if RTT is 250ms, there will a 250ms idle period between when one segment response is read and the next one starts. The obvious solution to this is to use HTTP pipelining, where a second request can be sent (on the persistent http/1.1 connection) before the first response is fully read. Unfortunately the way the http protocol is implemented in avformat makes implementing pipleining very complex. Instead, this commit simulates pipelining using two separate persistent http connections. This has the advantage of working independently of the http_persistent option, and can be used with http/1.0 servers as well. The pair of connections is swapped every time a new segment starts downloading, and a request for the next segment is sent on the secondary connection right away. This means the second response will be ready and waiting by the time the current response is fully read. Thanks, seems like an OK idea and the code seems straight-forward. Why is this a defaults-to-disabled option, instead of defaulting to enabled or just not having an option at all? I guess there may be a good reason, but I'd like to hear your thoughts on that. --- doc/demuxers.texi | 3 +++ libavformat/hls.c | 51 --- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/doc/demuxers.texi b/doc/demuxers.texi index 18ff7101ac..f2181fbb93 100644 --- a/doc/demuxers.texi +++ b/doc/demuxers.texi @@ -319,6 +319,9 @@ Default value is 1000. @item http_persistent Use persistent HTTP connections. Applicable only for HTTP streams. + +@item http_multiple +Use multiple HTTP connections for downloading HTTP segments. @end table @section image2 diff --git a/libavformat/hls.c b/libavformat/hls.c index f75c8f5eaa..487fa9a82f 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c [...] @@ -1426,11 +1440,20 @@ reload: if (ret) return ret; -ret = open_input(c, v, seg, >input); +if (c->http_multiple && v->input_next_requested) { +AVIOContext *tmp = v->input; +v->input = v->input_next; +v->input_next = tmp; Use FFSWAP(). [...] -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 3/5] avformat/hls: add http_persistent option
Hi! Aman Gupta kirjoitti 2017-12-13 02:35: From: Aman Gupta <a...@tmm1.net> This teaches the HLS demuxer to use the HTTP protocols multiple_requests=1 option, to take advantage of "Connection: Keep-Alive" when downloading playlists and segments from the HLS server. With the new option, you can avoid TCP connection and TLS negotiation overhead, which is particularly beneficial when streaming via a high-latency internet connection. Similar to the http_persistent option recently implemented in hlsenc.c Why is this implemented as an option instead of simply being always used by the demuxer? --- doc/demuxers.texi | 3 +++ libavformat/hls.c | 68 +++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/doc/demuxers.texi b/doc/demuxers.texi index 73dc0feec1..18ff7101ac 100644 --- a/doc/demuxers.texi +++ b/doc/demuxers.texi @@ -316,6 +316,9 @@ segment index to start live streams at (negative values are from the end). @item max_reload Maximum number of times a insufficient list is attempted to be reloaded. Default value is 1000. + +@item http_persistent +Use persistent HTTP connections. Applicable only for HTTP streams. @end table @section image2 diff --git a/libavformat/hls.c b/libavformat/hls.c index ab6ff187a6..5c1895c180 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -26,6 +26,7 @@ * http://tools.ietf.org/html/draft-pantos-http-live-streaming */ +#include "libavformat/http.h" #include "libavutil/avstring.h" #include "libavutil/avassert.h" #include "libavutil/intreadwrite.h" @@ -94,6 +95,7 @@ struct playlist { AVIOContext pb; uint8_t* read_buffer; AVIOContext *input; +int input_read_done; AVFormatContext *parent; int index; AVFormatContext *ctx; @@ -206,6 +208,8 @@ typedef struct HLSContext { int strict_std_compliance; char *allowed_extensions; int max_reload; +int http_persistent; +AVIOContext *playlist_pb; } HLSContext; static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) @@ -256,6 +260,7 @@ static void free_playlist_list(HLSContext *c) av_freep(>pb.buffer); if (pls->input) ff_format_io_close(c->ctx, >input); +pls->input_read_done = 0; if (pls->ctx) { pls->ctx->pb = NULL; avformat_close_input(>ctx); @@ -640,7 +645,24 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) return AVERROR_INVALIDDATA; -ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); +if (c->http_persistent && *pb && av_strstart(proto_name, "http", NULL)) { +URLContext *uc = ffio_geturlcontext(*pb); +av_assert0(uc); +(*pb)->eof_reached = 0; +ret = ff_http_do_new_request(uc, url); +if (ret == AVERROR_EXIT) { +ff_format_io_close(c->ctx, >playlist_pb); +return ret; +} else if (ret < 0) { +av_log(s, AV_LOG_WARNING, +"keepalive request failed for '%s', retrying with new connection: %s\n", +url, av_err2str(ret)); +ff_format_io_close(c->ctx, pb); +ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); +} +} else { +ret = s->io_open(s, pb, url, AVIO_FLAG_READ, ); +} if (ret >= 0) { // update cookies on http response with setcookies. char *new_cookies = NULL; @@ -683,10 +705,27 @@ static int parse_playlist(HLSContext *c, const char *url, char tmp_str[MAX_URL_SIZE]; struct segment *cur_init_section = NULL; +if (!in && c->http_persistent && c->playlist_pb) { +in = c->playlist_pb; +URLContext *uc = ffio_geturlcontext(in); +av_assert0(uc); +in->eof_reached = 0; +ret = ff_http_do_new_request(uc, url); +if (ret == AVERROR_EXIT) { +ff_format_io_close(c->ctx, >playlist_pb); +return ret; +} else if (ret < 0) { +av_log(c->ctx, AV_LOG_WARNING, +"keepalive request failed for '%s', retrying with new connection: %s\n", + url, av_err2str(ret)); +ff_format_io_close(c->ctx, >playlist_pb); +in = NULL; +} +} + The above code seems duplicated, please put it in a common function. [..] -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v4 2/2] lavf/hls: add option to defer parsing of variants
;playlists[0]->renditions, - >playlists[0]->n_renditions, - rend); +found = 0; +for (j = 0; j < var->playlists[0]->n_renditions; j++) { +if (var->playlists[0]->renditions[j] == rend) { +found = 1; +break; +} +} +if (!found) +dynarray_add(>playlists[0]->renditions, + >playlists[0]->n_renditions, + rend); +} } } [...] +static void activate_playlist(AVFormatContext *s, struct playlist *pls) { + + HLSContext *c = s->priv_data; + + if (pls->index < c->n_variants) { + + struct variant *var = c->variants[pls->index]; + + if (parse_playlist(c, pls->url, pls, NULL) < 0) + return; + if (var->audio_group[0]) + add_renditions_to_variant(c, var, AVMEDIA_TYPE_AUDIO, var->audio_group); + if (var->video_group[0]) + add_renditions_to_variant(c, var, AVMEDIA_TYPE_VIDEO, var->video_group); + if (var->subtitles_group[0]) + add_renditions_to_variant(c, var, AVMEDIA_TYPE_SUBTITLE, var->subtitles_group); Hmm, didn't notice this before, but I don't think these add_renditions_to_variant() calls use anything from the parse_playlist(media_pls) call so there should be no need to re-call them as the calls in hls_read_header() should have already worked perfectly. The video_group, audio_group, subtitles_group strings come from the master playlist which is always parsed in hls_read_header(). Then we could also drop all the changes you made in add_renditions_to_variant() as well as it would not be called more than once per variant. Or maybe I am missing something? Trying to keep variants, renditions, playlists, streams and programs straight in one's head does tend to cause headaches, after all... + init_playlist(c, pls); + } [...] -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hls demuxer: add option to defer parsing of variants
(Re-added ffmpeg-devel@) Rainer Hochecker kirjoitti 2017-12-03 09:16: 2017-12-02 16:09 GMT+01:00 Anssi Hannula <anssi.hann...@iki.fi>: Hi, Sorry about the delay. Rainer Hochecker kirjoitti 2017-11-28 00:23: 2017-11-27 22:53 GMT+01:00 Anssi Hannula <anssi.hann...@iki.fi>: Hi, Rainer Hochecker kirjoitti 2017-11-26 12:46: fixed mem leak poined out by Steven --- doc/demuxers.texi | 5 + libavformat/hls.c | 304 -- 2 files changed, 209 insertions(+), 100 deletions(-) [...] + +@item load_all_variants +If 0, only the first variant/playlist is loaded on open. All other variants +get disabled and can be enabled by setting discard option in program. +Default value is 1. @end table If playlist download+parsing is indeed taking long enough time that this is warranted, I wonder if we should maybe just never read any variant playlists in read_header(), and instead set AVFMTCTX_NOHEADER. Then the user may discard AVPrograms right after open, before unneeded playlists are loaded+parsed. This would avoid having a separate option/mode for this. Any thoughts? I actually tried it like this but somwhow did not like it because this is different to all other demuxers/formats. In general you open an input, call find_stream_info, and select a program. I did not like selecting the program between open and find_stream_info. OK I guess. Though arguably hls is already quite different from mpegts which is the only other demuxer that creates multiple AVPrograms. In the long run, I think I'd prefer the HLS demuxer to have a mode where only a single program/variant is given to the user at a time, with the demuxer handling the switching between the variants. But that would be a lot more work and I'm not even sure it would be feasible. So I guess your solution is OK, at least for now. Is there a reason the first variant/playlist is still parsed, i.e. why not simply not parse any media playlists (i.e. only master pls) when the new mode is selected? I thought this could become the new default after a while. Clients that don't select a program would still work. Selecting the first available program, if none was selected seems the most natural way. Selecting all or none make less sense. Hmm.. I wonder if selecting the highest-bandwidth-one might make more sense as the playlist/program order is arbitrary (IIRC) so currently an arbitrary playlist is parsed. If the player is selecting the variant/program based on bitrate (like Kodi does), then the first playlist would likely have been downloaded+parsed unnecessarily. Right, but this is acceptable. The majority of user don't have set network bandwidth anyway. If the user does not have bandwidth set, Kodi selects the highest-bandwidth program => so both the first playlist and the highest-bandwidth playlist get parsed. Also, even with your current patch you will need to set AVFMTCTX_NOHEADER as you defer avformat_new_stream() calls to read_packet(). I think you can update update_noheader_flag() to set flag_needed=1 if any pls is !pls->is_parsed. Actually I call open again after having set AVFMTCTX_NOHEADER. Could you please be more verbose on how you would like to have it? With your patch, with load_all_variants=0, this happens, if I follow the code right: 1. hls_read_header() - A single playlist is opened - in this case the subdemuxer has no AVFTMCTX_NOHEADER so AVFMTCTX_NOHEADER is not set on the main context. - Other playlists are not parsed. ... 2. user undiscards a program. 3. hls_read_packet() - Discard flag checking notices a new playlist is needed and it is parsed. - update_streams_from_subdemuxer() gets called from init_playlist(), and it calls avformat_new_stream() The read_packet documentation says: 'avformat_new_stream' can be called only if the flag * AVFMTCTX_NOHEADER is used and only in the calling thread (not in a * background thread). And you have not set AVFMTCTX_NOHEADER. So probably update_noheader_flag() needs this change: -if (pls->has_noheader_flag) { +if (pls->has_noheader_flag || !pls->is_parsed) { So that AVFMTCTX_NOHEADER gets always set if there are unparsed playlists left. That might cause e.g. avformat_find_stream_info (if it is called - looks like Kodi always does for HLS - which might be another thing to check to improve launch performance) to probe longer as it tries to find the missing streams in vain, though, so better check the performance is still OK afterwards... @section image2 diff --git a/libavformat/hls.c b/libavformat/hls.c index 786934af03..c42e0b0f95 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -112,6 +112,7 @@ struct playlist { int n_segments; struct segment **segments; int needed, cur_needed; +int parsed; int cur_seq_no; int64_t cur_seg_offset; int64_t last_load_time;
Re: [FFmpeg-devel] [PATCH] hls demuxer: add option to defer parsing of variants
Hi, Sorry about the delay. Rainer Hochecker kirjoitti 2017-11-28 00:23: 2017-11-27 22:53 GMT+01:00 Anssi Hannula <anssi.hann...@iki.fi>: Hi, Rainer Hochecker kirjoitti 2017-11-26 12:46: fixed mem leak poined out by Steven --- doc/demuxers.texi | 5 + libavformat/hls.c | 304 -- 2 files changed, 209 insertions(+), 100 deletions(-) [...] + +@item load_all_variants +If 0, only the first variant/playlist is loaded on open. All other variants +get disabled and can be enabled by setting discard option in program. +Default value is 1. @end table If playlist download+parsing is indeed taking long enough time that this is warranted, I wonder if we should maybe just never read any variant playlists in read_header(), and instead set AVFMTCTX_NOHEADER. Then the user may discard AVPrograms right after open, before unneeded playlists are loaded+parsed. This would avoid having a separate option/mode for this. Any thoughts? I actually tried it like this but somwhow did not like it because this is different to all other demuxers/formats. In general you open an input, call find_stream_info, and select a program. I did not like selecting the program between open and find_stream_info. OK I guess. Though arguably hls is already quite different from mpegts which is the only other demuxer that creates multiple AVPrograms. In the long run, I think I'd prefer the HLS demuxer to have a mode where only a single program/variant is given to the user at a time, with the demuxer handling the switching between the variants. But that would be a lot more work and I'm not even sure it would be feasible. So I guess your solution is OK, at least for now. Is there a reason the first variant/playlist is still parsed, i.e. why not simply not parse any media playlists (i.e. only master pls) when the new mode is selected? If the player is selecting the variant/program based on bitrate (like Kodi does), then the first playlist would likely have been downloaded+parsed unnecessarily. Also, even with your current patch you will need to set AVFMTCTX_NOHEADER as you defer avformat_new_stream() calls to read_packet(). I think you can update update_noheader_flag() to set flag_needed=1 if any pls is !pls->is_parsed. @section image2 diff --git a/libavformat/hls.c b/libavformat/hls.c index 786934af03..c42e0b0f95 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -112,6 +112,7 @@ struct playlist { int n_segments; struct segment **segments; int needed, cur_needed; +int parsed; int cur_seq_no; int64_t cur_seg_offset; int64_t last_load_time; @@ -206,6 +207,7 @@ typedef struct HLSContext { int strict_std_compliance; char *allowed_extensions; int max_reload; +int load_all_variants; } HLSContext; [...] @@ -721,6 +726,7 @@ static int parse_playlist(HLSContext *c, const char *url, free_segment_list(pls); pls->finished = 0; pls->type = PLS_TYPE_UNSPECIFIED; +pls->parsed = 1; } That "pls->parsed = 1" is in the "reinit old playlist for re-parse" block, is that intentional? I'd think it should rather be at the end of the function alongside setting pls->last_load_time, so it is set to 1 whenever parsing has been done. I put it at the beginning because I wanted to avoid that it gets tried again on error. OK. But now it is not set for master playlists, so maybe move pls->parsed = 1 below the "fail" label then? while (!avio_feof(in)) { read_chomp_line(in, line, sizeof(line)); [...] @@ -1631,6 +1655,124 @@ static int hls_close(AVFormatContext *s) return 0; } +static int init_playlist(HLSContext *c, struct playlist *pls) +{ This init_playlist() seems to be mostly code moved from below, could you split the patch so that the first patch moves the code around but does not change behavior, and the second patch makes the actual changes (i.e. adds the option)? That would ease e.g. future bisecting. Sure. At least I can try. [...] +return 0; +} + static int hls_read_header(AVFormatContext *s) { void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; @@ -1663,6 +1805,9 @@ static int hls_read_header(AVFormatContext *s) if ((ret = parse_playlist(c, s->filename, NULL, s->pb)) < 0) goto fail; +/* first playlist was created, set it to parsed */ +c->variants[0]->playlists[0]->parsed = 1; + I think parse_playlist() should set this when it parses something. That was pitfall for my v1. The first playlist gets parsed with no playlist given as argument. fate failed because non-master playlists were not set to parsed. I don't think I follow. If parse_playlist() would set the playlist as parsed on every call (whether failed or not), then all playlists would be set to parsed and this line wou
Re: [FFmpeg-devel] [PATCH] hls demuxer: add option to defer parsing of variants
Hi, Rainer Hochecker kirjoitti 2017-11-26 12:46: fixed mem leak poined out by Steven --- doc/demuxers.texi | 5 + libavformat/hls.c | 304 -- 2 files changed, 209 insertions(+), 100 deletions(-) [...] + +@item load_all_variants +If 0, only the first variant/playlist is loaded on open. All other variants +get disabled and can be enabled by setting discard option in program. +Default value is 1. @end table If playlist download+parsing is indeed taking long enough time that this is warranted, I wonder if we should maybe just never read any variant playlists in read_header(), and instead set AVFMTCTX_NOHEADER. Then the user may discard AVPrograms right after open, before unneeded playlists are loaded+parsed. This would avoid having a separate option/mode for this. Any thoughts? @section image2 diff --git a/libavformat/hls.c b/libavformat/hls.c index 786934af03..c42e0b0f95 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -112,6 +112,7 @@ struct playlist { int n_segments; struct segment **segments; int needed, cur_needed; +int parsed; int cur_seq_no; int64_t cur_seg_offset; int64_t last_load_time; @@ -206,6 +207,7 @@ typedef struct HLSContext { int strict_std_compliance; char *allowed_extensions; int max_reload; +int load_all_variants; } HLSContext; [...] @@ -721,6 +726,7 @@ static int parse_playlist(HLSContext *c, const char *url, free_segment_list(pls); pls->finished = 0; pls->type = PLS_TYPE_UNSPECIFIED; +pls->parsed = 1; } That "pls->parsed = 1" is in the "reinit old playlist for re-parse" block, is that intentional? I'd think it should rather be at the end of the function alongside setting pls->last_load_time, so it is set to 1 whenever parsing has been done. while (!avio_feof(in)) { read_chomp_line(in, line, sizeof(line)); [...] @@ -1631,6 +1655,124 @@ static int hls_close(AVFormatContext *s) return 0; } +static int init_playlist(HLSContext *c, struct playlist *pls) +{ This init_playlist() seems to be mostly code moved from below, could you split the patch so that the first patch moves the code around but does not change behavior, and the second patch makes the actual changes (i.e. adds the option)? That would ease e.g. future bisecting. [...] +return 0; +} + static int hls_read_header(AVFormatContext *s) { void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; @@ -1663,6 +1805,9 @@ static int hls_read_header(AVFormatContext *s) if ((ret = parse_playlist(c, s->filename, NULL, s->pb)) < 0) goto fail; +/* first playlist was created, set it to parsed */ +c->variants[0]->playlists[0]->parsed = 1; + I think parse_playlist() should set this when it parses something. if ((ret = save_avio_options(s)) < 0) goto fail; @@ -1675,8 +1820,15 @@ static int hls_read_header(AVFormatContext *s) goto fail; [...] /* Select the starting segments */ for (i = 0; i < c->n_playlists; i++) { struct playlist *pls = c->playlists[i]; -if (pls->n_segments == 0) +if (pls->n_segments == 0 && !pls->parsed) continue; Why? pls->cur_seq_no = select_cur_seq_no(c, pls); @@ -1736,97 +1892,9 @@ static int hls_read_header(AVFormatContext *s) /* Open the demuxer for each playlist */ for (i = 0; i < c->n_playlists; i++) { struct playlist *pls = c->playlists[i]; -AVInputFormat *in_fmt = NULL; - [...] -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support
Hi, 07.12.2016, 00:04, Franklin Phillips kirjoitti: > Assuming the reason why my patch wasn't being merged was because it > didn't use the X-TIMESTAMP-MAP, I have included the changes for that. > > Those changes were basically a merge of work done by > anssi.hann...@iki.fi which is why I've cc'd them. I'm sorry about the lack of response. I should be able to take a closer look by Friday this week, but with a quick look your read_packet_subtitle() seems to contain quite a lot of duplicated code with other parts of hls.c (unless I'm missing something), so some refactoring should be done. -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3 v2] avformat/hls: Add missing error check for avcodec_parameters_copy()
07.11.2016, 01:08, Andreas Cadhalpun kirjoitti: > On 06.11.2016 23:52, Anssi Hannula wrote: >> Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> >> --- >> >> 07.11.2016, 00:35, Andreas Cadhalpun kirjoitti: >>> On 06.11.2016 22:44, Anssi Hannula wrote: >>>> Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> >>>> --- >>>> libavformat/hls.c | 18 ++ >>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>> >>> >>> This misses checking the return code of the other occurrence of >>> set_stream_info_from_input_stream in hls_read_packet. >> >> Argh, true. Here's a new one. >> >> >> libavformat/hls.c | 27 +++++-- >> 1 file changed, 21 insertions(+), 6 deletions(-) >> > > LGTM. Thanks, all 3 applied to master and backported to 3.2 branch. -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3 v2] avformat/hls: Add missing error check for avcodec_parameters_copy()
Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- 07.11.2016, 00:35, Andreas Cadhalpun kirjoitti: > On 06.11.2016 22:44, Anssi Hannula wrote: >> Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> >> --- >> libavformat/hls.c | 18 ++ >> 1 file changed, 14 insertions(+), 4 deletions(-) >> > > This misses checking the return code of the other occurrence of > set_stream_info_from_input_stream in hls_read_packet. Argh, true. Here's a new one. libavformat/hls.c | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index ce52bf5..2bf86fa 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1528,9 +1528,13 @@ static void add_stream_to_programs(AVFormatContext *s, struct playlist *pls, AVS av_dict_set_int(>metadata, "variant_bitrate", bandwidth, 0); } -static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) +static int set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) { -avcodec_parameters_copy(st->codecpar, ist->codecpar); +int err; + +err = avcodec_parameters_copy(st->codecpar, ist->codecpar); +if (err < 0) +return err; if (pls->is_id3_timestamped) /* custom timestamps via id3 */ avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); @@ -1538,11 +1542,15 @@ static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); st->internal->need_context_update = 1; + +return 0; } /* add new subdemuxer streams to our context, if any */ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *pls) { +int err; + while (pls->n_main_streams < pls->ctx->nb_streams) { int ist_idx = pls->n_main_streams; AVStream *st = avformat_new_stream(s, NULL); @@ -1552,11 +1560,13 @@ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *p return AVERROR(ENOMEM); st->id = pls->index; -set_stream_info_from_input_stream(st, pls, ist); - dynarray_add(>main_streams, >n_main_streams, st); add_stream_to_programs(s, pls, st); + +err = set_stream_info_from_input_stream(st, pls, ist); +if (err < 0) +return err; } return 0; @@ -1990,8 +2000,13 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) /* There may be more situations where this would be useful, but this at least * handles newly probed codecs properly (i.e. request_probe by mpegts). */ -if (ist->codecpar->codec_id != st->codecpar->codec_id) -set_stream_info_from_input_stream(st, pls, ist); +if (ist->codecpar->codec_id != st->codecpar->codec_id) { +ret = set_stream_info_from_input_stream(st, pls, ist); +if (ret < 0) { +av_packet_unref(pkt); +return ret; +} +} return 0; } -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Fix probing mpegts audio streams that use probing
05.11.2016, 19:51, Andreas Cadhalpun kirjoitti: > On 05.11.2016 18:47, Andreas Cadhalpun wrote: >> On 05.11.2016 17:39, Anssi Hannula wrote: >>> @@ -1981,6 +1986,11 @@ static int hls_read_packet(AVFormatContext *s, >>> AVPacket *pkt) >>> >>> pls->ctx->streams[pls->pkt.stream_index]->time_base, >>> AV_TIME_BASE_Q); >>> >>> +/* There may be more situations where this would be useful, but >>> this at least >>> + * handles newly probed codecs properly (i.e. request_probe by >>> mpegts). */ >>> +if (ist->codecpar->codec_id != st->codecpar->codec_id) >>> +set_stream_info_from_input_stream(st, pls, ist); >> >> This has to set: >> ist->internal->need_context_update = 1; > ^ > Should have been 'st' not 'ist'. Thanks for the comments, followup is a new set with need_context_update = 1 added to set_stream_info_from_input_stream() and an additional patch to add error checks for the use of avcodec_parameters_copy(). Anssi Hannula (3): avformat/hls: Factor copying stream info to a separate function avformat/hls: Fix probing mpegts audio streams that use probing avformat/hls: Add missing error check for avcodec_parameters_copy() -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avformat/hls: Factor copying stream info to a separate function
Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- libavformat/hls.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index 3c09dd8..6fb652c 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1528,6 +1528,16 @@ static void add_stream_to_programs(AVFormatContext *s, struct playlist *pls, AVS av_dict_set_int(>metadata, "variant_bitrate", bandwidth, 0); } +static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) +{ +avcodec_parameters_copy(st->codecpar, ist->codecpar); + +if (pls->is_id3_timestamped) /* custom timestamps via id3 */ +avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); +else +avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); +} + /* add new subdemuxer streams to our context, if any */ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *pls) { @@ -1540,13 +1550,7 @@ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *p return AVERROR(ENOMEM); st->id = pls->index; - -avcodec_parameters_copy(st->codecpar, ist->codecpar); - -if (pls->is_id3_timestamped) /* custom timestamps via id3 */ -avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); -else -avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); +set_stream_info_from_input_stream(st, pls, ist); dynarray_add(>main_streams, >n_main_streams, st); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] avformat/hls: Add missing error check for avcodec_parameters_copy()
Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- libavformat/hls.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index ce52bf5..a744908 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1528,9 +1528,13 @@ static void add_stream_to_programs(AVFormatContext *s, struct playlist *pls, AVS av_dict_set_int(>metadata, "variant_bitrate", bandwidth, 0); } -static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) +static int set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) { -avcodec_parameters_copy(st->codecpar, ist->codecpar); +int err; + +err = avcodec_parameters_copy(st->codecpar, ist->codecpar); +if (err) +return err; if (pls->is_id3_timestamped) /* custom timestamps via id3 */ avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); @@ -1538,11 +1542,15 @@ static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); st->internal->need_context_update = 1; + +return 0; } /* add new subdemuxer streams to our context, if any */ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *pls) { +int err; + while (pls->n_main_streams < pls->ctx->nb_streams) { int ist_idx = pls->n_main_streams; AVStream *st = avformat_new_stream(s, NULL); @@ -1552,11 +1560,13 @@ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *p return AVERROR(ENOMEM); st->id = pls->index; -set_stream_info_from_input_stream(st, pls, ist); - dynarray_add(>main_streams, >n_main_streams, st); add_stream_to_programs(s, pls, st); + +err = set_stream_info_from_input_stream(st, pls, ist); +if (err) +return err; } return 0; -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/3 v2] avformat/hls: Fix probing mpegts audio streams that use probing
Commit 04964ac311abe670f ("avformat/hls: Fix missing streams in some cases with MPEG TS") caused a regression where subdemuxer streams that use probing (e.g. dts/eac3/mp2 in mpegts) no longer get probed properly. This is because the codec parameters from the subdemuxer stream, once probed, are not passed on to the main stream. Fix that by updating the codec parameters if the codec id changes. Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- v2: Added need_context_update = 1 and shortened the av_rescale_q() call to use the new ist pointer. libavformat/hls.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index 6fb652c..ce52bf5 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1536,6 +1536,8 @@ static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); else avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); + +st->internal->need_context_update = 1; } /* add new subdemuxer streams to our context, if any */ @@ -1950,6 +1952,8 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) /* If we got a packet, return it */ if (minplaylist >= 0) { struct playlist *pls = c->playlists[minplaylist]; +AVStream *ist; +AVStream *st; ret = update_streams_from_subdemuxer(s, pls); if (ret < 0) { @@ -1972,15 +1976,23 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) return AVERROR_BUG; } +ist = pls->ctx->streams[pls->pkt.stream_index]; +st = pls->main_streams[pls->pkt.stream_index]; + *pkt = pls->pkt; -pkt->stream_index = pls->main_streams[pls->pkt.stream_index]->index; +pkt->stream_index = st->index; reset_packet(>playlists[minplaylist]->pkt); if (pkt->dts != AV_NOPTS_VALUE) c->cur_timestamp = av_rescale_q(pkt->dts, - pls->ctx->streams[pls->pkt.stream_index]->time_base, +ist->time_base, AV_TIME_BASE_Q); +/* There may be more situations where this would be useful, but this at least + * handles newly probed codecs properly (i.e. request_probe by mpegts). */ +if (ist->codecpar->codec_id != st->codecpar->codec_id) +set_stream_info_from_input_stream(st, pls, ist); + return 0; } return AVERROR_EOF; -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Fix probing mpegts audio streams that use probing
05.11.2016, 19:27, Hendrik Leppkes kirjoitti: > On Sat, Nov 5, 2016 at 5:39 PM, Anssi Hannula <anssi.hann...@iki.fi> wrote: >> Commit 04964ac311abe670f ("avformat/hls: Fix missing streams in some >> cases with MPEG TS") caused a regression where subdemuxer streams that >> use probing (e.g. dts/eac3/mp2 in mpegts) no longer get probed properly. >> >> This is because the codec parameters from the subdemuxer stream, once >> probed, are not passed on to the main stream. >> >> Fix that by updating the codec parameters if the codec id changes. >> >> Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> >> --- >> libavformat/hls.c | 12 +++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/hls.c b/libavformat/hls.c >> index 6fb652c..8527f33 100644 >> --- a/libavformat/hls.c >> +++ b/libavformat/hls.c >> @@ -1950,6 +1950,8 @@ static int hls_read_packet(AVFormatContext *s, >> AVPacket *pkt) >> /* If we got a packet, return it */ >> if (minplaylist >= 0) { >> struct playlist *pls = c->playlists[minplaylist]; >> +AVStream *ist; >> +AVStream *st; >> >> ret = update_streams_from_subdemuxer(s, pls); >> if (ret < 0) { >> @@ -1972,8 +1974,11 @@ static int hls_read_packet(AVFormatContext *s, >> AVPacket *pkt) >> return AVERROR_BUG; >> } >> >> +ist = pls->ctx->streams[pls->pkt.stream_index]; >> +st = pls->main_streams[pls->pkt.stream_index]; >> + >> *pkt = pls->pkt; >> -pkt->stream_index = pls->main_streams[pls->pkt.stream_index]->index; >> +pkt->stream_index = st->index; >> reset_packet(>playlists[minplaylist]->pkt); >> >> if (pkt->dts != AV_NOPTS_VALUE) >> @@ -1981,6 +1986,11 @@ static int hls_read_packet(AVFormatContext *s, >> AVPacket *pkt) >> >> pls->ctx->streams[pls->pkt.stream_index]->time_base, >> AV_TIME_BASE_Q); >> >> +/* There may be more situations where this would be useful, but >> this at least >> + * handles newly probed codecs properly (i.e. request_probe by >> mpegts). */ >> +if (ist->codecpar->codec_id != st->codecpar->codec_id) >> +set_stream_info_from_input_stream(st, pls, ist); >> + > > A better solution would be checking > ist->internal->need_context_update, and also setting this in the > output stream when you copy params over, so the generic code knows > things changed. AFAICS that flag has already been handled & cleared by read_frame_internal() at this point. So I'll do what Andreas suggested and just set it to 1 here (or in set_stream_in_from_input_stream), unless I'm missing something... -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avformat/hls: Fix probing mpegts audio streams that use probing
Commit 04964ac311abe670f ("avformat/hls: Fix missing streams in some cases with MPEG TS") caused a regression where subdemuxer streams that use probing (e.g. dts/eac3/mp2 in mpegts) no longer get probed properly. This is because the codec parameters from the subdemuxer stream, once probed, are not passed on to the main stream. Fix that by updating the codec parameters if the codec id changes. Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- libavformat/hls.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index 6fb652c..8527f33 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1950,6 +1950,8 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) /* If we got a packet, return it */ if (minplaylist >= 0) { struct playlist *pls = c->playlists[minplaylist]; +AVStream *ist; +AVStream *st; ret = update_streams_from_subdemuxer(s, pls); if (ret < 0) { @@ -1972,8 +1974,11 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) return AVERROR_BUG; } +ist = pls->ctx->streams[pls->pkt.stream_index]; +st = pls->main_streams[pls->pkt.stream_index]; + *pkt = pls->pkt; -pkt->stream_index = pls->main_streams[pls->pkt.stream_index]->index; +pkt->stream_index = st->index; reset_packet(>playlists[minplaylist]->pkt); if (pkt->dts != AV_NOPTS_VALUE) @@ -1981,6 +1986,11 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt) pls->ctx->streams[pls->pkt.stream_index]->time_base, AV_TIME_BASE_Q); +/* There may be more situations where this would be useful, but this at least + * handles newly probed codecs properly (i.e. request_probe by mpegts). */ +if (ist->codecpar->codec_id != st->codecpar->codec_id) +set_stream_info_from_input_stream(st, pls, ist); + return 0; } return AVERROR_EOF; -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hls: call avformat_find_stream_info for mpegts subdemuxer
03.11.2016, 20:34, Andreas Cadhalpun kirjoitti: > On 03.11.2016 11:18, Anssi Hannula wrote: >> Andreas Cadhalpun kirjoitti 2016-11-03 02:12: >>> This fixes probing dts/eac3/mp2 in hls. >>> >>> The problem was introduced in commit >>> 04964ac311abe670fb3b60290a330f2067544b13. >>> >>> Also update the fate reference for the fate-segment-mp4-to-ts test. >> >> Thanks. >> >> Can you point me to some example streams with this issue? (or how to >> generate one) > > ffmpeg creates these, as I've mentioned in [1], e.g.: > $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a dca -f hls dca.hls -y &> > /dev/null > $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a eac3 -f hls eac3.hls -y &> > /dev/null > $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a mp2 -f hls mp2.hls -y &> > /dev/null > > Then run ffprobe on the created hls files to see the problems. > >> Unfortunately calling avformat_find_stream_info() for sub-mpegts streams is >> quite >> bandwidth-heavy (especially if there are many stream variants and/or >> alternative >> renditions, and especially now that we no longer clear mpegts >> AVFMTCTX_NOHEADER >> flag so that streams like #4930 work) so I tried to avoid that. >> It is better than doing nothing, though, if we can't find an alternative >> solution >> (better slow than not working). >> >> I'd first like to try to understand what is happening, though. > > OK. A better solution is always welcome. Thanks for the examples. The two follow-up patches fix them for me, can you confirm and/or see anything else? Anssi Hannula (2): avformat/hls: Factor copying stream info to a separate function avformat/hls: Fix probing mpegts audio streams that use probing -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] avformat/hls: Factor copying stream info to a separate function
Signed-off-by: Anssi Hannula <anssi.hann...@iki.fi> --- libavformat/hls.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/libavformat/hls.c b/libavformat/hls.c index 3c09dd8..6fb652c 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1528,6 +1528,16 @@ static void add_stream_to_programs(AVFormatContext *s, struct playlist *pls, AVS av_dict_set_int(>metadata, "variant_bitrate", bandwidth, 0); } +static void set_stream_info_from_input_stream(AVStream *st, struct playlist *pls, AVStream *ist) +{ +avcodec_parameters_copy(st->codecpar, ist->codecpar); + +if (pls->is_id3_timestamped) /* custom timestamps via id3 */ +avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); +else +avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); +} + /* add new subdemuxer streams to our context, if any */ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *pls) { @@ -1540,13 +1550,7 @@ static int update_streams_from_subdemuxer(AVFormatContext *s, struct playlist *p return AVERROR(ENOMEM); st->id = pls->index; - -avcodec_parameters_copy(st->codecpar, ist->codecpar); - -if (pls->is_id3_timestamped) /* custom timestamps via id3 */ -avpriv_set_pts_info(st, 33, 1, MPEG_TIME_BASE); -else -avpriv_set_pts_info(st, ist->pts_wrap_bits, ist->time_base.num, ist->time_base.den); +set_stream_info_from_input_stream(st, pls, ist); dynarray_add(>main_streams, >n_main_streams, st); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hls: call avformat_find_stream_info for mpegts subdemuxer
3600, 3600, 3600, 687, 0x00394b95, F=0x0, S=1,1, 0x00e000e0 +0, 7200, 10800, 3600, 445, 0x08c3d065, F=0x0, S=1,1, 0x00e000e0 +0, 10800, 28800, 3600, 4212, 0x56d12b8f, F=0x0, S=1,1, 0x00e000e0 +0, 14400, 21600, 3600, 1117, 0xd521260b, F=0x0, S=1,1, 0x00e000e0 +0, 18000, 18000, 3600, 892, 0x4262bdbc, F=0x0, S=1,1, 0x00e000e0 +0, 21600, 25200, 3600, 480, 0x3be1ef0b, F=0x0, S=1,1, 0x00e000e0 +0, 25200, 43200, 3600, 4065, 0x40dee237, F=0x0, S=1,1, 0x00e000e0 +0, 28800, 36000, 3600, 962, 0x31a4ceb1, F=0x0, S=1,1, 0x00e000e0 +0, 32400, 32400, 3600, 651, 0xb2aa317a, F=0x0, S=1,1, 0x00e000e0 +0, 36000, 39600, 3600, 543, 0x9c4e0024, F=0x0, S=1,1, 0x00e000e0 +0, 39600, 57600, 3600, 4221, 0x77c23977, F=0x0, S=1,1, 0x00e000e0 +0, 43200, 50400, 3600, 1040, 0x482cfa34, F=0x0, S=1,1, 0x00e000e0 +0, 46800, 46800, 3600, 576, 0x2686136a, F=0x0, S=1,1, 0x00e000e0 +0, 50400, 54000, 3600, 607, 0xc53c2339, F=0x0, S=1,1, 0x00e000e0 +0, 54000, 72000, 3600, 4755, 0x2f642b58, F=0x0, S=1,1, 0x00e000e0 +0, 57600, 64800, 3600, 1182, 0xbe1a4847, F=0x0, S=1,1, 0x00e000e0 +0, 61200, 61200, 3600, 809, 0x8d948a4e, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 68400, 3600, 656, 0x4fa03c2b, F=0x0 +0, 64800, 64800, 3600,22630, 0x9b109541, S=1, 1, 0x00e000e0 +0, 64800, 64800, 3600, 4021, 0xbf7cdb02, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 1096, 0x4f162690, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 687, 0x00394b95, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 445, 0x08c3d065, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 4212, 0x56d12b8f, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 1117, 0xd521260b, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 892, 0x4262bdbc, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 480, 0x3be1ef0b, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 4065, 0x40dee237, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 962, 0x31a4ceb1, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 651, 0xb2aa317a, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 543, 0x9c4e0024, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 4221, 0x77c23977, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 1040, 0x482cfa34, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 576, 0x2686136a, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 607, 0xc53c2339, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 72000, 3600, 4755, 0x2f642b58, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 1182, 0xbe1a4847, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 64800, 3600, 809, 0x8d948a4e, F=0x0, S=1,1, 0x00e000e0 +0, 64800, 68400, 3600, 656, 0x4fa03c2b, F=0x0, S=1,1, 0x00e000e0 +0, 68400, 86400, 3600,26555, 0x5629b584, S=1, 1, 0x00e000e0 +0, 72000, 79200, 3600, 1141, 0x761b31e8, F=0x0, S=1,1, 0x00e000e0 +0, 75600, 75600, 3600, 717, 0x57746351, F=0x0, S=1,1, 0x00e000e0 +0, 79200, 82800, 3600, 693, 0x78b24263, F=0x0, S=1,1, 0x00e000e0 +0, 82800, 100800, 3600, 3417, 0x560dbc89, F=0x0, S=1,1, 0x00e000e0 +0, 86400, 93600, 3600, 1128, 0xc0f1383c, F=0x0, S=1,1, 0x00e000e0 +0, 9, 9, 3600, 650, 0xc3ad485e, F=0x0, S=1,1, 0x00e000e0 +0, 93600, 97200, 3600, 766, 0xd3e9757d, F=0x0, S=1,1, 0x00e000e0 0, 97200, 115200, 3600, 4268, 0xec1235b5, F=0x0, S=1,1, 0x00e000e0 0, 100800, 108000, 3600, 1119, 0x65f51fb7, F=0x0, S=1,1, 0x00e000e0 0, 104400, 104400, 3600, 766, 0x213b78d3, F=0x0, S=1, 1, 0x00e000e0 -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
27.07.2016, 02:04, Michael Niedermayer kirjoitti: > On Tue, Jul 26, 2016 at 08:39:03PM +0300, Anssi Hannula wrote: >> 26.07.2016, 17:59, Michael Niedermayer kirjoitti: >>> On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: >>>> Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with >>>> AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed >>>> avformat_find_stream_info() to put the extradata it got from >>>> st->parser->parser->split() to st->internal->avctx instead of st->codec >>>> (from where it will be later copied to st->codecpar). >>>> >>>> However, in the same function, the "is stream ready?" check was changed >>>> to check for extradata in st->codecpar instead of st->codec. >>>> >>>> Extradata retrieved from split() is therefore not considered anymore, >>>> and avformat_find_stream_info() will therefore needlessly continue >>>> probing in some cases. >>>> >>>> Fix that by checking for the extradata at st->internal->avctx where it >>>> is actually put. >>>> >>>> --- >>>> >>>> Michael Niedermayer wrote: >>>>> seems to break fate here: >>>> [...] >>>> >>>> Oops, seems I messed up running fate and missed the "warning: only a >>>> subset of the fate tests will be run because SAMPLES is not specified" >>>> warning it gave... Thanks for catching that. >>>> >>>> Seems this reverse fix is actually needed, as extradata is actually >>>> copied in the other direction (from st->internal->avctx to >>>> st->codecpar). >>> >>> it seems this causes some changes, quite possibly thats simply due to >>> less packets being read >>> >>> libavformat/tests/seek >>> Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv >>> -duration 400 >>> with >>> http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv >> >> @@ -9,7 +9,7 @@ >> ret: 0 st:13 flags:1 dts: 86.75 pts: 86.75 pos: -1 >> size: 50436 >> ret:-1 st: 1 flags:0 ts: 250.577000 >> ret: 0 st: 1 flags:1 ts: 13.471000 >> -ret: 0 st:13 flags:1 dts: 5.909000 pts: 5.909000 pos: -1 >> size: 50436 >> +ret: 0 st:13 flags:1 dts: 1.776000 pts: 1.776000 pos: -1 >> size: 50436 >> ret:-1 st: 2 flags:0 ts: 176.365000 >> ret: 0 st: 2 flags:1 ts: 339.259000 >> ret: 0 st:13 flags:1 dts: 145.08 pts: 145.08 pos: >> -1 size: 50436 >> >> Seems to indeed be due to less packets being read in >> avformat_find_stream_info(). >> >> Matroska demuxer seems to add keyframes to the index as it encounters >> them, and in this case more keyframes are encountered in >> avformat_stream_info() phase, which seems to result in more accurate >> seeking in the early seconds of the file. >> >>> for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from >>> 1001/3 to 1/30 >> >> Looks like the timestamps are jittery in that sample: >> >> 36072.231911, dts = prev + 3002 >> 36072.265289, dts = prev + 3004 >> 36072.298656, dts = prev + 3003 >> 36072.331433, dts = prev + 2950 >> 36072.364800, dts = prev + 3003 >> 36072.398167, dts = prev + 3003 >> 36072.431533, dts = prev + 3003 >> 36072.464900, dts = prev + 3003 >> 36072.498267, dts = prev + 3003 >> 36072.531633, dts = prev + 3003 >> 36072.565000, dts = prev + 3003 >> 36072.598367, dts = prev + 3003 >> 36072.630667, dts = prev + 2907 >> 36072.664033, dts = prev + 3003 >> 36072.697400, dts = prev + 3003 >> 36072.730767, dts = prev + 3003 >> 36072.764133, dts = prev + 3003 >> 36072.797500, dts = prev + 3003 >> 36072.830867, dts = prev + 3003 >> 36072.864233, dts = prev + 3003 >> 36072.897600, dts = prev + 3003 >> 36072.939278, dts = prev + 3751 >> 36072.972644, dts = prev + 3003 >> 36073.006011, dts = prev + 3003 >> 36073.034478, dts = prev + 2562 >> 36073.067844, dts = prev + 3003 >> >> And this throws the ff_rfps_calculate() algorithm off unless it is >> called with fpsprobesize (fps_analyze_framecount) of 160 or more >> (default is 20). >> >> Not sure if something could/should be done to keep tbr of that file to >> be detected as 1001/3000. >> >> >> For both of these cases I verified that FFmpeg 3.0 had the same behavior >> as with this patch, i.e. their behavior was unintendedly changed by the >> original commit 6f69f7a8bf6a0d01. > > thanks alot for the deep analysis > i have no objections to the change > > > [...] Applied. -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
26.07.2016, 17:59, Michael Niedermayer kirjoitti: > On Tue, Jul 26, 2016 at 03:56:13PM +0300, Anssi Hannula wrote: >> Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with >> AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed >> avformat_find_stream_info() to put the extradata it got from >> st->parser->parser->split() to st->internal->avctx instead of st->codec >> (from where it will be later copied to st->codecpar). >> >> However, in the same function, the "is stream ready?" check was changed >> to check for extradata in st->codecpar instead of st->codec. >> >> Extradata retrieved from split() is therefore not considered anymore, >> and avformat_find_stream_info() will therefore needlessly continue >> probing in some cases. >> >> Fix that by checking for the extradata at st->internal->avctx where it >> is actually put. >> >> --- >> >> Michael Niedermayer wrote: >>> seems to break fate here: >> [...] >> >> Oops, seems I messed up running fate and missed the "warning: only a >> subset of the fate tests will be run because SAMPLES is not specified" >> warning it gave... Thanks for catching that. >> >> Seems this reverse fix is actually needed, as extradata is actually >> copied in the other direction (from st->internal->avctx to >> st->codecpar). > > it seems this causes some changes, quite possibly thats simply due to > less packets being read > > libavformat/tests/seek > Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv -duration > 400 > with > http://samples.ffmpeg.org/Matroska/matrix/Matrix.Reloaded.Trailer-640x346-XviD-1.0beta2-HE_AAC_subtitled.mkv @@ -9,7 +9,7 @@ ret: 0 st:13 flags:1 dts: 86.75 pts: 86.75 pos: -1 size: 50436 ret:-1 st: 1 flags:0 ts: 250.577000 ret: 0 st: 1 flags:1 ts: 13.471000 -ret: 0 st:13 flags:1 dts: 5.909000 pts: 5.909000 pos: -1 size: 50436 +ret: 0 st:13 flags:1 dts: 1.776000 pts: 1.776000 pos: -1 size: 50436 ret:-1 st: 2 flags:0 ts: 176.365000 ret: 0 st: 2 flags:1 ts: 339.259000 ret: 0 st:13 flags:1 dts: 145.08 pts: 145.08 pos: -1 size: 50436 Seems to indeed be due to less packets being read in avformat_find_stream_info(). Matroska demuxer seems to add keyframes to the index as it encounters them, and in this case more keyframes are encountered in avformat_stream_info() phase, which seems to result in more accurate seeking in the early seconds of the file. > for tickets//2254/ttvHD_vlc_sample.ts the tbr changes from > 1001/3 to 1/30 Looks like the timestamps are jittery in that sample: 36072.231911, dts = prev + 3002 36072.265289, dts = prev + 3004 36072.298656, dts = prev + 3003 36072.331433, dts = prev + 2950 36072.364800, dts = prev + 3003 36072.398167, dts = prev + 3003 36072.431533, dts = prev + 3003 36072.464900, dts = prev + 3003 36072.498267, dts = prev + 3003 36072.531633, dts = prev + 3003 36072.565000, dts = prev + 3003 36072.598367, dts = prev + 3003 36072.630667, dts = prev + 2907 36072.664033, dts = prev + 3003 36072.697400, dts = prev + 3003 36072.730767, dts = prev + 3003 36072.764133, dts = prev + 3003 36072.797500, dts = prev + 3003 36072.830867, dts = prev + 3003 36072.864233, dts = prev + 3003 36072.897600, dts = prev + 3003 36072.939278, dts = prev + 3751 36072.972644, dts = prev + 3003 36073.006011, dts = prev + 3003 36073.034478, dts = prev + 2562 36073.067844, dts = prev + 3003 And this throws the ff_rfps_calculate() algorithm off unless it is called with fpsprobesize (fps_analyze_framecount) of 160 or more (default is 20). Not sure if something could/should be done to keep tbr of that file to be detected as 1001/3000. For both of these cases I verified that FFmpeg 3.0 had the same behavior as with this patch, i.e. their behavior was unintendedly changed by the original commit 6f69f7a8bf6a0d01. > are these in intended / expected ? > > I can confirm that this reduces the amount of data read in many files -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avio starting offset and hls regression in 3.1
25.07.2016, 22:13, Anssi Hannula kirjoitti: > Hi all, > > Commit d0fc5de3a643fe7f974ed14e410c2ac2f4147d7e [1] merged in > commit 81306fd4bdeb5c17d4db771e4fec684773b5790f "hls: eliminate ffurl_* > usage" from libav, which changed the hls demuxer to use AVIOContext > instead of URLContext for its HTTP requests. > > HLS demuxer uses the "offset" option for the http demuxer, requesting > the initial file offset for the I/O (http URLProtocol uses the "Range:" > HTTP header to try to accommodate that). > > However, the code in libavformat/aviobuf.c seems to be doing its own > accounting for the current file offset (AVIOContext.pos), with the > assumption that the initial offset is always zero. > > HLS demuxer does an explicit seek after open_url to account for cases > where the "offset" was not effective (due to the URL being a local file > or the HTTP server not obeying it), which should be a no-op in case the > file offset is already at that position. > > However, since aviobuf.c code thinks the starting offset is 0, this > doesn't work properly. > > E.g. this command, which worked fine in FFmpeg 3.0, now fails after > approx. 10 seconds of playback: > ffplay > https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8 > > > Now, what should be done to fix this regression? > > I guess there are at least these options: > > 1. Make aviobuf.c call seek() to check the offset after open: > > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -913,6 +913,14 @@ int ffio_fdopen(AVIOContext **s, URLContext *h) > (*s)->read_pause = io_read_pause; > (*s)->read_seek = io_read_seek; > } > + > +/* get initial buffer position (may be non-zero with e.g. offset > option for http) */ > +if ((*s)->seek) { > +int64_t pos = (*s)->seek((*s)->opaque, 0, SEEK_CUR); > +if (pos >= 0) > +(*s)->pos = pos; > +} > + > (*s)->av_class = _avio_class; > return 0; > fail: > > I guess that has a high chance of further regressions (*if* it is even > correct), though, as I guess there may be seek() handlers that don't > have SEEK_CUR = 0 as a no-op. > > 2. Revert the commit. I'm not familiar enough (or I've just forgotten > that stuff) with avio/ffurl to know how big the benefit of using avio_ > instead of ffurl_ is, though... > > 3. Drop the seek call from HLS demuxer in case protocol is http*, and > assume the HTTP server obeyed the ranged request. After thinking about it a bit, I think I'll go with this 3rd one to fix the regression, unless a better idea surfaces. It is quite trivial to implement, and server obeying ranged requests is mandatory anyway in HLS specs when byte-ranged segments are used - the failure mode in the non-spec-compliant will just be a bit different. > > Any other ideas / opinions for the course of action? > > > [1] https://github.com/FFmpeg/FFmpeg/commit/d0fc5de3a643fe7f974ed14 > -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed avformat_find_stream_info() to put the extradata it got from st->parser->parser->split() to st->internal->avctx instead of st->codec (from where it will be later copied to st->codecpar). However, in the same function, the "is stream ready?" check was changed to check for extradata in st->codecpar instead of st->codec. Extradata retrieved from split() is therefore not considered anymore, and avformat_find_stream_info() will therefore needlessly continue probing in some cases. Fix that by checking for the extradata at st->internal->avctx where it is actually put. --- Michael Niedermayer wrote: > seems to break fate here: [...] Oops, seems I messed up running fate and missed the "warning: only a subset of the fate tests will be run because SAMPLES is not specified" warning it gave... Thanks for catching that. Seems this reverse fix is actually needed, as extradata is actually copied in the other direction (from st->internal->avctx to st->codecpar). Fate passes here now. libavformat/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index e5a99ff..5a902ea 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -3432,7 +3432,7 @@ FF_ENABLE_DEPRECATION_WARNINGS break; } if (st->parser && st->parser->parser->split && -!st->codecpar->extradata) +!st->internal->avctx->extradata) break; if (st->first_dts == AV_NOPTS_VALUE && !(ic->iformat->flags & AVFMT_NOTIMESTAMPS) && -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/utils: Fix find_stream_info not considering the extradata it found
Commit 9200514ad8717c6 ("lavf: replace AVStream.codec with AVStream.codecpar") merged in commit 6f69f7a8bf6a0d01 changed avformat_find_stream_info() to put the extradata it got from st->parser->parser->split() to avctx instead of st->codec. However, in the same function, the "is stream ready?" check was changed to check for extradata in st->codecpar instead of st->codec. Extradata retrieved from split() is therefore not considered anymore, and avformat_find_stream_info() will therefore needlessly continue probing in some cases. Fix that by putting the extradata in st->codecpar, from where it will be copied to avctx at the end of avformat_find_stream_info() along with all other parameters. --- I'm not overly familiar with this code, so review accordingly :) This can be reproduced e.g. with a simple sample generated with "ffmpeg -t 20 -f lavfi -i cellauto out.ts", where ffprobe will analyze for 7 seconds (mpegts max_stream_analyze_duration) instead of the expected 5 seconds (max_analyze_duration). The difference will be more dramatic with streams that trigger the "stop find_stream_info from waiting for more streams when all programs have received a PMT" check in mpegts demuxer. libavformat/utils.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index e5a99ff..392143e 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -3581,16 +3581,16 @@ FF_ENABLE_DEPRECATION_WARNINGS if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) ff_rfps_add_frame(ic, st, pkt->dts); #endif -if (st->parser && st->parser->parser->split && !avctx->extradata) { +if (st->parser && st->parser->parser->split && !st->codecpar->extradata) { int i = st->parser->parser->split(avctx, pkt->data, pkt->size); if (i > 0 && i < FF_MAX_EXTRADATA_SIZE) { -avctx->extradata_size = i; -avctx->extradata = av_mallocz(avctx->extradata_size + - AV_INPUT_BUFFER_PADDING_SIZE); -if (!avctx->extradata) +st->codecpar->extradata_size = i; +st->codecpar->extradata = av_mallocz(st->codecpar->extradata_size + + AV_INPUT_BUFFER_PADDING_SIZE); +if (!st->codecpar->extradata) return AVERROR(ENOMEM); -memcpy(avctx->extradata, pkt->data, - avctx->extradata_size); +memcpy(st->codecpar->extradata, pkt->data, + st->codecpar->extradata_size); } } -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] avio starting offset and hls regression in 3.1
Hi all, Commit d0fc5de3a643fe7f974ed14e410c2ac2f4147d7e [1] merged in commit 81306fd4bdeb5c17d4db771e4fec684773b5790f "hls: eliminate ffurl_* usage" from libav, which changed the hls demuxer to use AVIOContext instead of URLContext for its HTTP requests. HLS demuxer uses the "offset" option for the http demuxer, requesting the initial file offset for the I/O (http URLProtocol uses the "Range:" HTTP header to try to accommodate that). However, the code in libavformat/aviobuf.c seems to be doing its own accounting for the current file offset (AVIOContext.pos), with the assumption that the initial offset is always zero. HLS demuxer does an explicit seek after open_url to account for cases where the "offset" was not effective (due to the URL being a local file or the HTTP server not obeying it), which should be a no-op in case the file offset is already at that position. However, since aviobuf.c code thinks the starting offset is 0, this doesn't work properly. E.g. this command, which worked fine in FFmpeg 3.0, now fails after approx. 10 seconds of playback: ffplay https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8 Now, what should be done to fix this regression? I guess there are at least these options: 1. Make aviobuf.c call seek() to check the offset after open: --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -913,6 +913,14 @@ int ffio_fdopen(AVIOContext **s, URLContext *h) (*s)->read_pause = io_read_pause; (*s)->read_seek = io_read_seek; } + +/* get initial buffer position (may be non-zero with e.g. offset option for http) */ +if ((*s)->seek) { +int64_t pos = (*s)->seek((*s)->opaque, 0, SEEK_CUR); +if (pos >= 0) +(*s)->pos = pos; +} + (*s)->av_class = _avio_class; return 0; fail: I guess that has a high chance of further regressions (*if* it is even correct), though, as I guess there may be seek() handlers that don't have SEEK_CUR = 0 as a no-op. 2. Revert the commit. I'm not familiar enough (or I've just forgotten that stuff) with avio/ffurl to know how big the benefit of using avio_ instead of ffurl_ is, though... 3. Drop the seek call from HLS demuxer in case protocol is http*, and assume the HTTP server obeyed the ranged request. Any other ideas / opinions for the course of action? [1] https://github.com/FFmpeg/FFmpeg/commit/d0fc5de3a643fe7f974ed14 -- Anssi Hannula ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel