Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support

[Franklin Phillips]

Sorry for the test failure, I didn't realise there were tests for HLS
demuxer because the tests don't have HLS in their names.

I am sending new patches which don't break the tests.

___
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

[Michael Niedermayer]

On Fri, Dec 30, 2016 at 08:10:05PM +, Franklin Phillips wrote:
> Each subtile segment is a WebVTT file and needs to be demuxed
> separately. These segments also contain a header to synchronize their
> timing with the MPEG TS stream so those timestamps are requested from
> the WebVTT demuxer through an AVOption.
> 
> Signed-off-by: Franklin Phillips 

this breaks fate

--- ./tests/ref/fate/segment-mp4-to-ts  2016-12-30 00:31:20.420965026 +0100
+++ tests/data/fate/segment-mp4-to-ts   2017-01-01 02:49:40.011075738 +0100
@@ -24,7 +24,7 @@
 0,  54000,  72000,0, 4755, 0x2f642b58, F=0x0, S=1,
1, 0x00e000e0
 0,  57600,  64800,0, 1182, 0xbe1a4847, F=0x0, S=1,
1, 0x00e000e0
 0,  61200,  61200,0,  809, 0x8d948a4e, F=0x0, S=1,
1, 0x00e000e0
-0,  64800,  68400,0,  656, 0x4fa03c2b, F=0x0, S=1,
1, 0x00e000e0
+0,  64800,  68400,0,  656, 0x4fa03c2b, F=0x0
 0,  68400,  86400,0,26555, 0x5629b584, S=1,1, 
0x00e000e0
 0,  72000,  79200,0, 1141, 0x761b31e8, F=0x0, S=1,
1, 0x00e000e0
 0,  75600,  75600,0,  717, 0x57746351, F=0x0, S=1,
1, 0x00e000e0
@@ -48,7 +48,7 @@
 0, 140400, 158400, 3600, 5328, 0xd2c55ac6, F=0x0, S=1,
1, 0x00e000e0
 0, 144000, 151200, 3600, 1271, 0x46006870, F=0x0, S=1,
1, 0x00e000e0
 0, 147600, 147600, 3600,  849, 0x94dc99c7, F=0x0, S=1,
1, 0x00e000e0
-0, 151200, 154800, 3600,  753, 0xf4236cab, F=0x0, S=1,
1, 0x00e000e0
+0, 151200, 154800, 3600,  753, 0xf4236cab, F=0x0
 0, 154800, 172800, 3600,25825, 0xd5464dee, S=1,1, 
0x00e000e0
 0, 158400, 165600, 3600, 1206, 0x8ce84344, F=0x0, S=1,
1, 0x00e000e0
 0, 162000, 162000, 3600,  867, 0x312fa07d, F=0x0, S=1,
1, 0x00e000e0
@@ -96,7 +96,7 @@
 0, 313200, 331200, 3600, 5352, 0x59997996, F=0x0, S=1,
1, 0x00e000e0
 0, 316800, 324000, 3600, 1501, 0xb3b8f001, F=0x0, S=1,
1, 0x00e000e0
 0, 320400, 320400, 3600,  941, 0x92b0cb18, F=0x0, S=1,
1, 0x00e000e0
-0, 324000, 327600, 3600,  823, 0x3d548355, F=0x0, S=1,
1, 0x00e000e0
+0, 324000, 327600, 3600,  823, 0x3d548355, F=0x0
 0, 327600, 345600, 3600,24042, 0x441e94fb, S=1,1, 
0x00e000e0
 0, 331200, 338400, 3600, 1582, 0x4f5d1049, F=0x0, S=1,
1, 0x00e000e0
 0, 334800, 334800, 3600,  945, 0x4f3cc9e8, F=0x0, S=1,
1, 0x00e000e0
@@ -120,7 +120,7 @@
 0, 399600, 417600, 3600, 1862, 0x22a2a06c, F=0x0, S=1,
1, 0x00e000e0
 0, 403200, 410400, 3600,  359, 0x11bdae52, F=0x0, S=1,
1, 0x00e000e0
 0, 406800, 406800, 3600,  235, 0xbec26964, F=0x0, S=1,
1, 0x00e000e0
-0, 410400, 414000, 3600,  221, 0x8380682c, F=0x0, S=1,
1, 0x00e000e0
+0, 410400, 414000, 3600,  221, 0x8380682c, F=0x0
 0, 414000, 432000, 3600,22588, 0xf0ecf072, S=1,1, 
0x00e000e0
 0, 417600, 424800, 3600,  383, 0x4f3bb571, F=0x0, S=1,
1, 0x00e000e0
 0, 421200, 421200, 3600,  257, 0x22e87802, F=0x0, S=1,
1, 0x00e000e0
Test segment-mp4-to-ts failed. Look at tests/data/fate/segment-mp4-to-ts.err 
for details.
make: *** [fate-segment-mp4-to-ts] Error 1
TESTsegment-adts-to-mkv
--- ./tests/ref/fate/segment-adts-to-mkv-header-all 2016-12-30 
00:31:20.420965026 +0100
+++ tests/data/fate/segment-adts-to-mkv 2017-01-01 02:49:40.047075739 +0100
@@ -21,21 +21,3 @@
 0,832,832,   64,  147, 0x226043d7
 0,896,896,   64,  119, 0x8ad931ed
 0,960,960,   64,  153, 0xbb6e432f
-0,   1024,   1024,   64,  185, 0xa01f4ff3
-0,   1088,   1088,   64,  126, 0x85503ce6
-0,   1152,   1152,   64,  246, 0x652c7b59
-0,   1216,   1216,   64,  162, 0xc9f04da0
-0,   1280,   1280,   64,  135, 0x71fa3be0
-0,   1344,   1344,   64,  246, 0x7a6f7788
-0,   1408,   1408,   64,  262, 0xd3097781
-0,   1472,   1472,   64,   60, 0x09a118f5
-0,   1536,   1536,   64,  255, 0xbab5793c
-0,   1600,   1600,   64,  153, 0x6b6a44fb
-0,   1664,   1664,   64,  160, 0x550e4530
-0,   1728,   1728,   64,  215, 0x7fe66144
-0,   1792,   1792,   64,  144, 0xcd723f7d
-0,   1856,   1856,   64,  187, 0x2a0b5c1b
-0,   1920,   1920,   64,  177, 0xb8c355d5
-0,   1984,   1984,   64,  156, 0x867d4f3a
-0,   2048,   2048,  

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support

[Franklin Phillips]

On Fri, Dec 16, 2016 at 05:13:50PM +0100, wm4 wrote:
> On Thu, 15 Dec 2016 22:01:46 +
> Franklin Phillips  wrote:
> 
> > Hi,
> > 
> > I tested this code by converting HLS streams into mp4 files and it
> > seemed to work fine. However I recently compiled mpv with these changes
> > and when I play back the Apple example stream given in ticket #2833, it
> > seems to skip a lot of subtitles. It works fine for the real world
> > streams I'm using it for but it's probably worth holding off with this
> > change until that's worked out.
> 
> Could be a mpv-specific issue, maybe report it there.
I think you might be right, I could not reproduce it with ffmpeg alone
but I also cannot report it until these changes have been merged.
> 
> > Also I tried to make minimal changes to the existing code path because
> > there don't seem to be any tests for HLS making it hard to know when
> > something is broken which I guess is why there is the duplication.
> > However I think you are right and it should be refactored, I had a
> > further look at how it could be done and will continue working on it.
> 
> I wonder why not. Couldn't we put a small HLS stream on disk for FATE
> and let libavformat demux it? This still wouldn't test HTTP interaction
> of course.
This sounds like a good idea.

I have refactored the code to reduce duplication of code and sent the
new patches.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support

[Franklin Phillips]

Each subtile segment is a WebVTT file and needs to be demuxed
separately. These segments also contain a header to synchronize their
timing with the MPEG TS stream so those timestamps are requested from
the WebVTT demuxer through an AVOption.

Signed-off-by: Franklin Phillips 
---
 libavformat/hls.c | 196 ++
 1 file changed, 138 insertions(+), 58 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 3ae3c7c..278c3b2 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -153,6 +153,8 @@ struct playlist {
  * playlist, if any. */
 int n_init_sections;
 struct segment **init_sections;
+
+int is_subtitle; /* Indicates if it's a subtitle playlist */
 };
 
 /*
@@ -203,6 +205,7 @@ typedef struct HLSContext {
 char *headers;   ///< holds HTTP headers set as an 
AVOption to the HTTP protocol context
 char *http_proxy;///< holds the address of the HTTP 
proxy server
 AVDictionary *avio_opts;
+AVDictionary *demuxer_opts;
 int strict_std_compliance;
 } HLSContext;
 
@@ -309,6 +312,8 @@ static struct playlist *new_playlist(HLSContext *c, const 
char *url,
 ff_make_absolute_url(pls->url, sizeof(pls->url), base, url);
 pls->seek_timestamp = AV_NOPTS_VALUE;
 
+pls->is_subtitle = 0;
+
 pls->is_id3_timestamped = -1;
 pls->id3_mpegts_timestamp = AV_NOPTS_VALUE;
 
@@ -482,11 +487,6 @@ static struct rendition *new_rendition(HLSContext *c, 
struct rendition_info *inf
 if (type == AVMEDIA_TYPE_SUBTITLE && !info->uri[0])
 return NULL;
 
-/* TODO: handle subtitles (each segment has to parsed separately) */
-if (c->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL)
-if (type == AVMEDIA_TYPE_SUBTITLE)
-return NULL;
-
 rend = av_mallocz(sizeof(struct rendition));
 if (!rend)
 return NULL;
@@ -501,9 +501,14 @@ static struct rendition *new_rendition(HLSContext *c, 
struct rendition_info *inf
 /* add the playlist if this is an external rendition */
 if (info->uri[0]) {
 rend->playlist = new_playlist(c, info->uri, url_base);
-if (rend->playlist)
+if (rend->playlist) {
 dynarray_add(>playlist->renditions,
  >playlist->n_renditions, rend);
+if (type == AVMEDIA_TYPE_SUBTITLE) {
+rend->playlist->is_subtitle = 1;
+rend->playlist->is_id3_timestamped = 0;
+}
+}
 }
 
 if (info->assoc_language[0]) {
@@ -1241,8 +1246,103 @@ static int read_data(void *opaque, uint8_t *buf, int 
buf_size)
 {
 struct playlist *v = opaque;
 HLSContext *c = v->parent->priv_data;
-int ret, i;
-int just_opened = 0;
+int ret, just_opened = 0;
+struct segment *seg;
+
+if (v->cur_seq_no - v->start_seq_no >= v->n_segments) {
+return AVERROR_EOF;
+} else {
+seg = current_segment(v);
+}
+
+if (!v->input) {
+/* load/update Media Initialization Section, if any */
+ret = update_init_section(v, seg);
+if (ret < 0)
+return ret;
+
+ret = open_input(c, v, seg);
+if (ret < 0) {
+if (ff_check_interrupt(c->interrupt_callback))
+return AVERROR_EXIT;
+av_log(v->parent, AV_LOG_WARNING, "Failed to open segment of 
playlist %d\n",
+   v->index);
+return ret;
+}
+just_opened = 1;
+}
+
+if (v->init_sec_buf_read_offset < v->init_sec_data_len) {
+/* Push init section out first before first actual segment */
+int copy_size = FFMIN(v->init_sec_data_len - 
v->init_sec_buf_read_offset, buf_size);
+memcpy(buf, v->init_sec_buf, copy_size);
+v->init_sec_buf_read_offset += copy_size;
+return copy_size;
+}
+
+ret = read_from_url(v, seg, buf, buf_size, READ_NORMAL);
+if (ret > 0) {
+if (just_opened && v->is_id3_timestamped != 0) {
+/* Intercept ID3 tags here, elementary audio streams are required
+ * to convey timestamps using them in the beginning of each 
segment. */
+intercept_id3(v, buf, buf_size, );
+}
+}
+
+return ret;
+}
+
+static int nested_io_open(AVFormatContext *s, AVIOContext **pb, const char 
*url,
+  int flags, AVDictionary **opts)
+{
+av_log(s, AV_LOG_ERROR,
+   "A HLS playlist item '%s' referred to an external file '%s'. "
+   "Opening this file was forbidden for security reasons\n",
+   s->filename, url);
+return AVERROR(EPERM);
+}
+
+static int init_subtitle_context(struct playlist *pls)
+{
+HLSContext *c = pls->parent->priv_data;
+AVInputFormat *in_fmt;
+AVDictionary *opts = NULL;
+int ret = 0;
+
+if (!(pls->ctx = avformat_alloc_context()))
+return AVERROR(ENOMEM);
+
+pls->read_buffer = 

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support

[wm4]

On Thu, 15 Dec 2016 22:01:46 +
Franklin Phillips  wrote:

> Hi,
> 
> I tested this code by converting HLS streams into mp4 files and it
> seemed to work fine. However I recently compiled mpv with these changes
> and when I play back the Apple example stream given in ticket #2833, it
> seems to skip a lot of subtitles. It works fine for the real world
> streams I'm using it for but it's probably worth holding off with this
> change until that's worked out.

Could be a mpv-specific issue, maybe report it there.

> Also I tried to make minimal changes to the existing code path because
> there don't seem to be any tests for HLS making it hard to know when
> something is broken which I guess is why there is the duplication.
> However I think you are right and it should be refactored, I had a
> further look at how it could be done and will continue working on it.

I wonder why not. Couldn't we put a small HLS stream on disk for FATE
and let libavformat demux it? This still wouldn't test HTTP interaction
of course.

> If you have any advice for testing the current HLS functionality so I
> can be confident that there are no regressions, I would appreciate that.

___
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

[Franklin Phillips]

Hi,

I tested this code by converting HLS streams into mp4 files and it
seemed to work fine. However I recently compiled mpv with these changes
and when I play back the Apple example stream given in ticket #2833, it
seems to skip a lot of subtitles. It works fine for the real world
streams I'm using it for but it's probably worth holding off with this
change until that's worked out.

Also I tried to make minimal changes to the existing code path because
there don't seem to be any tests for HLS making it hard to know when
something is broken which I guess is why there is the duplication.
However I think you are right and it should be refactored, I had a
further look at how it could be done and will continue working on it.

If you have any advice for testing the current HLS functionality so I
can be confident that there are no regressions, I would appreciate that.

Thanks
___
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

[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 2/2] avformat/hls: Add subtitle support

[Franklin Phillips]

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avformat/hls: Add subtitle support

[Franklin Phillips]

Each subtile segment is a WebVTT file and needs to be demuxed
separately. These segments also contain a header to synchronize their
timing with the MPEG TS stream so those timestamps are requested from
the WebVTT demuxer through an AVOption.

Signed-off-by: Franklin Phillips 
---
 libavformat/hls.c | 197 --
 1 file changed, 177 insertions(+), 20 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 3ae3c7c..7f1a55e 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -153,6 +153,8 @@ struct playlist {
  * playlist, if any. */
 int n_init_sections;
 struct segment **init_sections;
+
+int is_subtitle; /* Indicates if the playlist is for subtitles */
 };
 
 /*
@@ -203,6 +205,7 @@ typedef struct HLSContext {
 char *headers;   ///< holds HTTP headers set as an 
AVOption to the HTTP protocol context
 char *http_proxy;///< holds the address of the HTTP 
proxy server
 AVDictionary *avio_opts;
+AVDictionary *demuxer_opts;
 int strict_std_compliance;
 } HLSContext;
 
@@ -312,6 +315,8 @@ static struct playlist *new_playlist(HLSContext *c, const 
char *url,
 pls->is_id3_timestamped = -1;
 pls->id3_mpegts_timestamp = AV_NOPTS_VALUE;
 
+pls->is_subtitle = 0;
+
 dynarray_add(>playlists, >n_playlists, pls);
 return pls;
 }
@@ -482,11 +487,6 @@ static struct rendition *new_rendition(HLSContext *c, 
struct rendition_info *inf
 if (type == AVMEDIA_TYPE_SUBTITLE && !info->uri[0])
 return NULL;
 
-/* TODO: handle subtitles (each segment has to parsed separately) */
-if (c->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL)
-if (type == AVMEDIA_TYPE_SUBTITLE)
-return NULL;
-
 rend = av_mallocz(sizeof(struct rendition));
 if (!rend)
 return NULL;
@@ -501,9 +501,14 @@ static struct rendition *new_rendition(HLSContext *c, 
struct rendition_info *inf
 /* add the playlist if this is an external rendition */
 if (info->uri[0]) {
 rend->playlist = new_playlist(c, info->uri, url_base);
-if (rend->playlist)
+if (rend->playlist) {
 dynarray_add(>playlist->renditions,
  >playlist->n_renditions, rend);
+if (type == AVMEDIA_TYPE_SUBTITLE) {
+rend->playlist->is_subtitle = 1;
+rend->playlist->is_id3_timestamped = 0;
+}
+}
 }
 
 if (info->assoc_language[0]) {
@@ -1349,6 +1354,146 @@ reload:
 goto restart;
 }
 
+static int nested_io_open(AVFormatContext *s, AVIOContext **pb, const char 
*url,
+  int flags, AVDictionary **opts)
+{
+av_log(s, AV_LOG_ERROR,
+   "A HLS playlist item '%s' referred to an external file '%s'. "
+   "Opening this file was forbidden for security reasons\n",
+   s->filename, url);
+return AVERROR(EPERM);
+}
+
+static int read_data_simple(void *opaque, uint8_t *buf, int buf_size)
+{
+struct playlist *v = opaque;
+HLSContext *c = v->parent->priv_data;
+struct segment *seg;
+
+if (v->cur_seq_no >= v->start_seq_no + v->n_segments) {
+return AVERROR_EOF;
+} else {
+seg = current_segment(v);
+}
+
+if (!v->input) {
+int ret = open_input(c, v, seg);
+if (ret < 0) {
+if (ff_check_interrupt(c->interrupt_callback))
+return AVERROR_EXIT;
+av_log(v->parent, AV_LOG_WARNING, "Failed to open segment of 
playlist %d\n",
+   v->index);
+return ret;
+}
+}
+
+return read_from_url(v, seg, buf, buf_size, READ_NORMAL);
+}
+
+static int read_packet_subtitle(struct playlist *v, AVPacket *pkt)
+{
+HLSContext *c = v->parent->priv_data;
+int ret, i;
+
+restart:
+if (!v->needed)
+return AVERROR_EOF;
+
+if (!v->input) {
+int64_t reload_interval;
+
+/* Check that the playlist is still needed before opening a new
+ * segment. */
+if (v->ctx && v->ctx->nb_streams) {
+v->needed = 0;
+for (i = 0; i < v->n_main_streams; i++) {
+if (v->main_streams[i]->discard < AVDISCARD_ALL) {
+v->needed = 1;
+break;
+}
+}
+}
+if (!v->needed) {
+av_log(v->parent, AV_LOG_INFO, "No longer receiving playlist %d\n",
+v->index);
+return AVERROR_EOF;
+}
+
+/* If this is a live stream and the reload interval has elapsed since
+ * the last playlist reload, reload the playlists now. */
+reload_interval = default_reload_interval(v);
+
+if (!v->finished &&
+av_gettime_relative() - v->last_load_time >= reload_interval) {
+if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) {
+av_log(v->parent,