diff mbox series

[FFmpeg-devel,v2] avformat/webvttdec, enc: correctly process files containing STYLE, REGION blocks

Message ID CA+_k4RJ1CNriPbhxxzLf1O6V1VuP2OXch3E8LqOsMNSRUyEnTw@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avformat/webvttdec, enc: correctly process files containing STYLE, REGION blocks | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make warning Make failed

Commit Message

Dave Evans Oct. 13, 2020, 9:25 a.m. UTC 
This patch fixes the total failure to parse cues when style and region
definition blocks are contained in the input file, and ensures those
blocks are written to the output when copying.

The sample attached needs to be added to samples at the path shown in
the patch in order to validate that the original issue is fixed.

Same as v1 except the test has been changed as requested in the original review.

Cheers,
Dave
Subject: [PATCH] avformat/webvttdec,enc: correctly decode files containing
 STYLE, REGION blocks

Add the ability to extract cues from files containing one or more WebVTT
region definition block or WebVTT style block. Previously the decoder would
misinterpret these and fail to parse any cues at all. Also add the ability to
write these, or alternative header information, back out in the encoder using
the codec extradata.
---
 libavformat/webvttdec.c                   | 16 ++++++-
 libavformat/webvttenc.c                   | 12 ++++++
 tests/fate/subtitles.mak                  |  3 ++
 tests/ref/fate/sub-webvtt-styleandregions | 51 +++++++++++++++++++++++
 4 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 tests/ref/fate/sub-webvtt-styleandregions

Comments

Dave Evans March 16, 2021, 9:23 p.m. UTC | #1 
Would it be possible for someone to take a look at this please? Would be
lovely to get it merged at some point. Please let me know if further
changes are needed.

Cheers,
Dave

On Tue, Oct 13, 2020 at 10:25 AM Dave Evans <dave.evans@m2amedia.tv> wrote:

> This patch fixes the total failure to parse cues when style and region
> definition blocks are contained in the input file, and ensures those
> blocks are written to the output when copying.
>
> The sample attached needs to be added to samples at the path shown in
> the patch in order to validate that the original issue is fixed.
>
> Same as v1 except the test has been changed as requested in the original
> review.
>
> Cheers,
> Dave
>
Dave Evans Nov. 24, 2021, 4:04 p.m. UTC | #2 
Giving this another go. It would be really great to get webvtt with style
and region blocks working correctly in ffmpeg. This patch would solve at
least #9064, probably #8684 and possibly other tickets.

Please let me know if further changes are needed.

Cheers,
Dave



On Tue, Mar 16, 2021 at 9:23 PM Dave Evans <dave.evans@m2amedia.tv> wrote:

> Would it be possible for someone to take a look at this please? Would be
> lovely to get it merged at some point. Please let me know if further
> changes are needed.
>
> Cheers,
> Dave
>
> On Tue, Oct 13, 2020 at 10:25 AM Dave Evans <dave.evans@m2amedia.tv>
> wrote:
>
>> This patch fixes the total failure to parse cues when style and region
>> definition blocks are contained in the input file, and ensures those
>> blocks are written to the output when copying.
>>
>> The sample attached needs to be added to samples at the path shown in
>> the patch in order to validate that the original issue is fixed.
>>
>> Same as v1 except the test has been changed as requested in the original
>> review.
>>
>> Cheers,
>> Dave
>>
>
diff mbox series

Patch

diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
index 8d2fdfed37..c6cc367383 100644
--- a/libavformat/webvttdec.c
+++ b/libavformat/webvttdec.c
@@ -60,7 +60,7 @@  static int64_t read_ts(const char *s)
 static int webvtt_read_header(AVFormatContext *s)
 {
     WebVTTContext *webvtt = s->priv_data;
-    AVBPrint cue;
+    AVBPrint cue, header;
     int res = 0;
     AVStream *st = avformat_new_stream(s, NULL);
 
@@ -72,6 +72,7 @@  static int webvtt_read_header(AVFormatContext *s)
     st->disposition |= webvtt->kind;
 
     av_bprint_init(&cue,    0, AV_BPRINT_SIZE_UNLIMITED);
+    av_bprint_init(&header, 0, AV_BPRINT_SIZE_UNLIMITED);
 
     for (;;) {
         int i;
@@ -89,12 +90,18 @@  static int webvtt_read_header(AVFormatContext *s)
         p = identifier = cue.str;
         pos = avio_tell(s->pb);
 
-        /* ignore header chunk */
+        /* ignore the magic word and any comments */
         if (!strncmp(p, "\xEF\xBB\xBFWEBVTT", 9) ||
             !strncmp(p, "WEBVTT", 6) ||
             !strncmp(p, "NOTE", 4))
             continue;
 
+        /* store the style and region blocks from the header */
+        if (!strncmp(p, "STYLE", 5) || !strncmp(p, "REGION", 6)) {
+            av_bprintf(&header, "%s%s", header.len ? "\n\n" : "", p);
+            continue;
+        }
+
         /* optional cue identifier (can be a number like in SRT or some kind of
          * chaptering id) */
         for (i = 0; p[i] && p[i] != '\n' && p[i] != '\r'; i++) {
@@ -161,12 +168,17 @@  static int webvtt_read_header(AVFormatContext *s)
         SET_SIDE_DATA(settings,   AV_PKT_DATA_WEBVTT_SETTINGS);
     }
 
+    res = ff_bprint_to_codecpar_extradata(st->codecpar, &header);
+    if (res < 0)
+        goto end;
+
     ff_subtitles_queue_finalize(s, &webvtt->q);
 
 end:
     if (res < 0)
         ff_subtitles_queue_clean(&webvtt->q);
     av_bprint_finalize(&cue,    NULL);
+    av_bprint_finalize(&header, NULL);
     return res;
 }
 
diff --git a/libavformat/webvttenc.c b/libavformat/webvttenc.c
index cbd989dcb6..fcbd3ee10a 100644
--- a/libavformat/webvttenc.c
+++ b/libavformat/webvttenc.c
@@ -58,6 +58,18 @@  static int webvtt_write_header(AVFormatContext *ctx)
 
     avio_printf(pb, "WEBVTT\n");
 
+    if (par->extradata_size > 0) {
+        size_t header_size = par->extradata_size;
+
+        if (par->extradata[0] != '\n')
+            avio_printf(pb, "\n");
+
+        avio_write(pb, par->extradata, header_size);
+
+        if (par->extradata[header_size - 1] != '\n')
+            avio_printf(pb, "\n");
+    }
+
     return 0;
 }
 
diff --git a/tests/fate/subtitles.mak b/tests/fate/subtitles.mak
index 6323d0f93d..375f81ef93 100644
--- a/tests/fate/subtitles.mak
+++ b/tests/fate/subtitles.mak
@@ -91,6 +91,9 @@  fate-sub-webvtt: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/WebVTT_capability_
 FATE_SUBTITLES_ASS-$(call DEMDEC, WEBVTT, WEBVTT) += fate-sub-webvtt2
 fate-sub-webvtt2: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/WebVTT_extended_tester.vtt
 
+FATE_SUBTITLES-$(call ALLYES, WEBVTT_DEMUXER, WEBVTT_MUXER) += fate-sub-webvtt-styleandregions
+fate-sub-webvtt-styleandregions: CMD = fmtstdout webvtt -i $(TARGET_SAMPLES)/sub/webvtt_style_and_regions.vtt -c:s copy
+
 FATE_SUBTITLES-$(call ALLYES, SRT_DEMUXER SUBRIP_DECODER WEBVTT_ENCODER WEBVTT_MUXER) += fate-sub-webvttenc
 fate-sub-webvttenc: CMD = fmtstdout webvtt -i $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt
 
diff --git a/tests/ref/fate/sub-webvtt-styleandregions b/tests/ref/fate/sub-webvtt-styleandregions
new file mode 100644
index 0000000000..8c8776adab
--- /dev/null
+++ b/tests/ref/fate/sub-webvtt-styleandregions
@@ -0,0 +1,51 @@ 
+WEBVTT
+
+REGION
+id:son
+width:40%
+lines:3
+regionanchor:20%,80%
+viewportanchor:20%,80%
+scroll:up
+
+REGION
+id:father
+width:40%
+lines:3
+regionanchor:80%,80%
+viewportanchor:80%,80%
+scroll:up
+
+STYLE
+::cue(i) {
+  /* make i tags italic */
+  font-style: italic
+}
+
+STYLE
+::cue(v[voice="Son"]) {
+  color: magenta
+}
+
+STYLE
+::cue(v[voice="Father"]) {
+  color: yellow
+}
+
+00:10.000 --> 00:25.000 region:son align:left
+<v Son>Can I tell you a joke, Dad?
+
+00:12.500 --> 00:27.500 region:father align:right
+<v Father>Sure, I could do with a laugh.
+
+00:15.000 --> 00:30.000 region:son align:left
+<v Son>Where do sheep go to get their hair cut?
+
+00:17.500 --> 00:32.500 region:father align:right
+<v Father>I don't know, son. Where do sheep go to get their hair cut?
+
+00:20.000 --> 00:35.000 region:son align:left
+<v Son>To the baa-baa shop!
+
+00:22.500 --> 00:37.500 region:father align:right
+<v Father><i>[facepalms]</i>