[FFmpeg-devel] [PATCH] avformat/spdifenc: handle long TrueHD input_timing gaps

2020-04-12 Thread Anssi Hannula 
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

2020-02-20 Thread Anssi Hannula 
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

2018-10-15 Thread Anssi Hannula 

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

2018-10-14 Thread Anssi Hannula 
---

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

2018-04-03 Thread Anssi Hannula 

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

2017-12-17 Thread Anssi Hannula 

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

2017-12-17 Thread Anssi Hannula 

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

2017-12-17 Thread Anssi Hannula 

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

2017-12-09 Thread Anssi Hannula 
;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

2017-12-09 Thread Anssi Hannula 

(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

2017-12-02 Thread Anssi Hannula 

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

2017-11-27 Thread Anssi Hannula 

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

2016-12-12 Thread Anssi Hannula 
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()

2016-11-07 Thread Anssi Hannula 
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()

2016-11-06 Thread Anssi Hannula 
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

2016-11-06 Thread Anssi Hannula 
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

2016-11-06 Thread Anssi Hannula 
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()

2016-11-06 Thread Anssi Hannula 
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

2016-11-06 Thread Anssi Hannula 
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

2016-11-06 Thread Anssi Hannula 
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

2016-11-05 Thread Anssi Hannula 
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

2016-11-05 Thread Anssi Hannula 
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

2016-11-05 Thread Anssi Hannula 
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

2016-11-03 Thread Anssi Hannula 
 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

2016-07-27 Thread Anssi Hannula 
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

2016-07-26 Thread Anssi Hannula 
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

2016-07-26 Thread Anssi Hannula 
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

2016-07-26 Thread Anssi Hannula 
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

2016-07-26 Thread Anssi Hannula 
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

2016-07-25 Thread Anssi Hannula 
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