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 |
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 |
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
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".
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 --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} };