diff mbox series

[FFmpeg-devel] Add matroska codec id S_TEXT/WEBVTT for WebVTT streams.

Message ID 20210207035518.404429-2-wwong5545@gmail.com
State New
Headers show
Series [FFmpeg-devel] Add matroska codec id S_TEXT/WEBVTT for WebVTT streams. | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Walter Feb. 7, 2021, 3:55 a.m. UTC 
Added the codec id S_TEXT/WEBVTT in order to bring ffmpeg generated files
closer to the matroska spec. The list of matroska codec ids was also
rearranged to push the old codec id (D_WEBVTT/SUBTITLES) to the bottom of
the list so that S_TEXT/WEBVTT is the default codec id for new mkv files
with webvtt subtitles.

Fixes ticket #5641: http://trac.ffmpeg.org/ticket/5641
---
 libavformat/matroska.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Andreas Rheinhardt Feb. 7, 2021, 8:08 a.m. UTC | #1 
Walter Wong:
> Added the codec id S_TEXT/WEBVTT in order to bring ffmpeg generated files
> closer to the matroska spec. The list of matroska codec ids was also
> rearranged to push the old codec id (D_WEBVTT/SUBTITLES) to the bottom of
> the list so that S_TEXT/WEBVTT is the default codec id for new mkv files
> with webvtt subtitles.
> 
> Fixes ticket #5641: http://trac.ffmpeg.org/ticket/5641
> ---
>  libavformat/matroska.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/matroska.c b/libavformat/matroska.c
> index 7c56aba403..1128c96451 100644
> --- a/libavformat/matroska.c
> +++ b/libavformat/matroska.c
> @@ -60,16 +60,12 @@ const CodecTags ff_mkv_codec_tags[]={
>      {"A_VORBIS"         , AV_CODEC_ID_VORBIS},
>      {"A_WAVPACK4"       , AV_CODEC_ID_WAVPACK},
>  
> -    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},
> -    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},
> -    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},
> -    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},
> -
>      {"S_TEXT/UTF8"      , AV_CODEC_ID_SUBRIP},
>      {"S_TEXT/UTF8"      , AV_CODEC_ID_TEXT},
>      {"S_TEXT/ASCII"     , AV_CODEC_ID_TEXT},
>      {"S_TEXT/ASS"       , AV_CODEC_ID_ASS},
>      {"S_TEXT/SSA"       , AV_CODEC_ID_ASS},
> +    {"S_TEXT/WEBVTT"    , AV_CODEC_ID_WEBVTT},
>      {"S_ASS"            , AV_CODEC_ID_ASS},
>      {"S_SSA"            , AV_CODEC_ID_ASS},
>      {"S_VOBSUB"         , AV_CODEC_ID_DVD_SUBTITLE},
> @@ -100,6 +96,14 @@ const CodecTags ff_mkv_codec_tags[]={
>      {"V_VP8"            , AV_CODEC_ID_VP8},
>      {"V_VP9"            , AV_CODEC_ID_VP9},
>  
> +    /* These codec strings for webvtt seem to be depreciated from the Matroska
> +       spec, but it might be useful to keep them for compatibility for older
> +       files. These are at the bottom so they get matched after S_TEXT/WEBVTT. */
> +    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},
> +    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},
> +    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},
> +    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},
> +
>      {""                 , AV_CODEC_ID_NONE}
>  };
>  
> 
You are not fixing #5641 at all; in fact your comment suggests that you
misunderstand the problem: There are two ways to mux WebVTT, one for
WebM (using these "D_WEBVTT_*" values and one for Matroska. The WebM way
of doing WebVTT is the older one of the two and it is horrible, but it
is the one that is supported. There was a patchset last year [1], [2] to
fix this and implement the Matroska way, yet the author never sent a
follow-up.

Your patch would make the muxer write the packets in the WebM way, yet
the header would claim it to be the Matroska way. This is worse than it
is now.

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264695.html
[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264693.html
Walter Feb. 7, 2021, 8:23 a.m. UTC | #2 
I understand. Would it be better to roll back the patch to just adding S_TEXT/WEBVTT to list of recognized codec ids for now?
-------- Original message --------From: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> Date: 2021-02-07  3:08 AM  (GMT-05:00) To: ffmpeg-devel@ffmpeg.org Subject: Re: [FFmpeg-devel] [PATCH] Add matroska codec id S_TEXT/WEBVTT for WebVTT streams. Walter Wong:> Added the codec id S_TEXT/WEBVTT in order to bring ffmpeg generated files> closer to the matroska spec. The list of matroska codec ids was also> rearranged to push the old codec id (D_WEBVTT/SUBTITLES) to the bottom of> the list so that S_TEXT/WEBVTT is the default codec id for new mkv files> with webvtt subtitles.> > Fixes ticket #5641: http://trac.ffmpeg.org/ticket/5641> --->  libavformat/matroska.c | 14 +++++++++----->  1 file changed, 9 insertions(+), 5 deletions(-)> > diff --git a/libavformat/matroska.c b/libavformat/matroska.c> index 7c56aba403..1128c96451 100644> --- a/libavformat/matroska.c> +++ b/libavformat/matroska.c> @@ -60,16 +60,12 @@ const CodecTags ff_mkv_codec_tags[]={>      {"A_VORBIS"         , AV_CODEC_ID_VORBIS},>      {"A_WAVPACK4"       , AV_CODEC_ID_WAVPACK},>  > -    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},> -    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},> -    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},> -    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},> ->      {"S_TEXT/UTF8"      , AV_CODEC_ID_SUBRIP},>      {"S_TEXT/UTF8"      , AV_CODEC_ID_TEXT},>      {"S_TEXT/ASCII"     , AV_CODEC_ID_TEXT},>      {"S_TEXT/ASS"       , AV_CODEC_ID_ASS},>      {"S_TEXT/SSA"       , AV_CODEC_ID_ASS},> +    {"S_TEXT/WEBVTT"    , AV_CODEC_ID_WEBVTT},>      {"S_ASS"            , AV_CODEC_ID_ASS},>      {"S_SSA"            , AV_CODEC_ID_ASS},>      {"S_VOBSUB"         , AV_CODEC_ID_DVD_SUBTITLE},> @@ -100,6 +96,14 @@ const CodecTags ff_mkv_codec_tags[]={>      {"V_VP8"            , AV_CODEC_ID_VP8},>      {"V_VP9"            , AV_CODEC_ID_VP9},>  > +    /* These codec strings for webvtt seem to be depreciated from the Matroska> +       spec, but it might be useful to keep them for compatibility for older> +       files. These are at the bottom so they get matched after S_TEXT/WEBVTT. */> +    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},> +    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},> +    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},> +    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},> +>      {""                 , AV_CODEC_ID_NONE}>  };>  > You are not fixing #5641 at all; in fact your comment suggests that youmisunderstand the problem: There are two ways to mux WebVTT, one forWebM (using these "D_WEBVTT_*" values and one for Matroska. The WebM wayof doing WebVTT is the older one of the two and it is horrible, but itis the one that is supported. There was a patchset last year [1], [2] tofix this and implement the Matroska way, yet the author never sent afollow-up.Your patch would make the muxer write the packets in the WebM way, yetthe header would claim it to be the Matroska way. This is worse than itis now.- Andreas[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264695.html[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264693.html_______________________________________________ffmpeg-devel mailing listffmpeg-devel@ffmpeg.orghttps://ffmpeg.org/mailman/listinfo/ffmpeg-develTo unsubscribe, visit link above, or emailffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Feb. 7, 2021, 8:30 a.m. UTC | #3 
Walter:
> I understand. Would it be better to roll back the patch to just adding S_TEXT/WEBVTT to list of recognized codec ids for now?

We would need to add support for the Matroska way first.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/matroska.c b/libavformat/matroska.c
index 7c56aba403..1128c96451 100644
--- a/libavformat/matroska.c
+++ b/libavformat/matroska.c
@@ -60,16 +60,12 @@  const CodecTags ff_mkv_codec_tags[]={
     {"A_VORBIS"         , AV_CODEC_ID_VORBIS},
     {"A_WAVPACK4"       , AV_CODEC_ID_WAVPACK},
 
-    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},
-    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},
-    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},
-    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},
-
     {"S_TEXT/UTF8"      , AV_CODEC_ID_SUBRIP},
     {"S_TEXT/UTF8"      , AV_CODEC_ID_TEXT},
     {"S_TEXT/ASCII"     , AV_CODEC_ID_TEXT},
     {"S_TEXT/ASS"       , AV_CODEC_ID_ASS},
     {"S_TEXT/SSA"       , AV_CODEC_ID_ASS},
+    {"S_TEXT/WEBVTT"    , AV_CODEC_ID_WEBVTT},
     {"S_ASS"            , AV_CODEC_ID_ASS},
     {"S_SSA"            , AV_CODEC_ID_ASS},
     {"S_VOBSUB"         , AV_CODEC_ID_DVD_SUBTITLE},
@@ -100,6 +96,14 @@  const CodecTags ff_mkv_codec_tags[]={
     {"V_VP8"            , AV_CODEC_ID_VP8},
     {"V_VP9"            , AV_CODEC_ID_VP9},
 
+    /* These codec strings for webvtt seem to be depreciated from the Matroska
+       spec, but it might be useful to keep them for compatibility for older
+       files. These are at the bottom so they get matched after S_TEXT/WEBVTT. */
+    {"D_WEBVTT/SUBTITLES"   , AV_CODEC_ID_WEBVTT},
+    {"D_WEBVTT/CAPTIONS"    , AV_CODEC_ID_WEBVTT},
+    {"D_WEBVTT/DESCRIPTIONS", AV_CODEC_ID_WEBVTT},
+    {"D_WEBVTT/METADATA"    , AV_CODEC_ID_WEBVTT},
+
     {""                 , AV_CODEC_ID_NONE}
 };