diff mbox series

[FFmpeg-devel,1/3] libavcodec: Make disposition forced work with mov_text subtitles.

Message ID 20220718160833.12397-2-facefunk@fcfnk.com
State New
Headers show
Series [FFmpeg-devel,1/3] libavcodec: Make disposition forced work with mov_text subtitles. | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

facefunk July 18, 2022, 4:08 p.m. UTC 
We are not currently able to force mov_text subtitles by setting -disposition:s:0 +forced or equivalent. By setting the forced flags in movtextenc as specifid in https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html subtitles can be forced as expected in VLC and similar players.

Copy AVStream disposition to AVCodecContext and use to set DISPLAY_FLAG_ALL_SAMPLES_FORCED in movtextenc.c.

Signed-off-by: facefunk <facefunk@fcfnk.com>
---
 fftools/ffmpeg_opt.c    |  5 +++++
 libavcodec/avcodec.h    | 16 ++++++++++++++++
 libavcodec/movtextenc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 61 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer July 18, 2022, 4:57 p.m. UTC | #1 
On Mon, Jul 18, 2022 at 05:08:31PM +0100, facefunk wrote:
> We are not currently able to force mov_text subtitles by setting -disposition:s:0 +forced or equivalent. By setting the forced flags in movtextenc as specifid in https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html subtitles can be forced as expected in VLC and similar players.
> 
> Copy AVStream disposition to AVCodecContext and use to set DISPLAY_FLAG_ALL_SAMPLES_FORCED in movtextenc.c.
> 
> Signed-off-by: facefunk <facefunk@fcfnk.com>
> ---
>  fftools/ffmpeg_opt.c    |  5 +++++
>  libavcodec/avcodec.h    | 16 ++++++++++++++++
>  libavcodec/movtextenc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index e08455478f..d8ea1d4bc1 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -2244,6 +2244,11 @@ static int set_dispositions(OutputFile *of, AVFormatContext *ctx)
>  
>              if (ret < 0)
>                  return ret;
> +
> +            // For output streams, copy stream disposition to the AVCodecContext
> +            // object.
> +            if(ost->enc_ctx)
> +                ost->enc_ctx->stream_disposition = ost->st->disposition;
>          }
>      } else {
>          // For each media type with more than one stream, find a suitable stream to

> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index cb5c25bf63..eb11de0293 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -37,6 +37,8 @@
>  #include "libavutil/pixfmt.h"
>  #include "libavutil/rational.h"
>  
> +#include "libavformat/avformat.h"

libavcodec should not depend on libavformat


> +
>  #include "codec.h"
>  #include "codec_desc.h"
>  #include "codec_par.h"
> @@ -2054,6 +2056,20 @@ typedef struct AVCodecContext {
>       *             The decoder can then override during decoding as needed.
>       */
>      AVChannelLayout ch_layout;
> +
> +    /**
> +     * Stream disposition - a combination of AV_DISPOSITION_* flags from
> +     * libavformat.
> +     *
> +     * Copied from the relevant AVStream object for codecs that need access to
> +     * the stream disposition parameter, such as movtextenc.c which needs to
> +     * read the AV_DISPOSITION_FORCED flag so it knows when to set forced
> +     * subtitles.
> +     *
> +     * - encoding: Set by set_dispositions in ffmpeg_opt.c.

ffmpeg is one application that uses libavcodec. 
libavcodec API should not really talk about ffmpeg doing this or that, thats
kind of odd

also, your patches dont contain a full name, thats ok if thats what you want
but if you want your full name in git then you may want to correct that

thx


[...]
facefunk July 18, 2022, 8 p.m. UTC | #2 
Okay, great! Thanks for reviewing.

I've addressed your points as well as a few tweaks in the spirit of preserving the abstraction boundary.

Would the attached patch revision be acceptable?
diff mbox series

Patch

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index e08455478f..d8ea1d4bc1 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -2244,6 +2244,11 @@  static int set_dispositions(OutputFile *of, AVFormatContext *ctx)
 
             if (ret < 0)
                 return ret;
+
+            // For output streams, copy stream disposition to the AVCodecContext
+            // object.
+            if(ost->enc_ctx)
+                ost->enc_ctx->stream_disposition = ost->st->disposition;
         }
     } else {
         // For each media type with more than one stream, find a suitable stream to
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index cb5c25bf63..eb11de0293 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -37,6 +37,8 @@ 
 #include "libavutil/pixfmt.h"
 #include "libavutil/rational.h"
 
+#include "libavformat/avformat.h"
+
 #include "codec.h"
 #include "codec_desc.h"
 #include "codec_par.h"
@@ -2054,6 +2056,20 @@  typedef struct AVCodecContext {
      *             The decoder can then override during decoding as needed.
      */
     AVChannelLayout ch_layout;
+
+    /**
+     * Stream disposition - a combination of AV_DISPOSITION_* flags from
+     * libavformat.
+     *
+     * Copied from the relevant AVStream object for codecs that need access to
+     * the stream disposition parameter, such as movtextenc.c which needs to
+     * read the AV_DISPOSITION_FORCED flag so it knows when to set forced
+     * subtitles.
+     *
+     * - encoding: Set by set_dispositions in ffmpeg_opt.c.
+     * - decoding: unused
+     */
+    int stream_disposition;
 } AVCodecContext;
 
 /**
diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
index 728338f2cc..d2550585e8 100644
--- a/libavcodec/movtextenc.c
+++ b/libavcodec/movtextenc.c
@@ -50,6 +50,40 @@ 
 #define FONTSIZE_SCALE(s,fs) ((fs) * (s)->font_scale_factor + 0.5)
 #define av_bprint_append_any(buf, data, size)   av_bprint_append_data(buf, ((const char*)data), size)
 
+/**
+ * https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html
+ *
+ * Display flags
+ * A 32-bit integer containing flags that describe how the subtitle text should
+ * be drawn.
+ * The following flags are defined:
+ *
+ * Vertical placement
+ * Controls vertical placement of the subtitle text.
+ * If this flag is set, the subtitle media handler uses the top coordinate of
+ * the display bounds of the override 'tbox' text box to determine the
+ * subtitle’s vertical placement as described in Subtitle Track Header Size
+ * and Placement. Otherwise, the subtitle displays at the bottom of the video.
+ */
+#define DISPLAY_FLAG_VERTICAL_PLACEMENT_TOP 0x20000000
+
+/**
+ * Some samples are forced
+ * Indicates whether any subtitle samples contain forced atoms. If this flag is
+ * set, at least one sample contains a forced ('frcd') atom as described in
+ * Subtitle Sample Data.
+ */
+#define DISPLAY_FLAG_SOME_SAMPLES_FORCED    0x40000000
+
+/**
+ * All samples are forced
+ * If this flag is set, the subtitle media handler treats all samples as forced
+ * subtitles, regardless of the presence or absence of a 'frcd' atom.
+ * If this flag is set, the Some Samples Are Forced flag must also be set
+ * (making 0xC0000000).
+ */
+#define DISPLAY_FLAG_ALL_SAMPLES_FORCED     0x80000000
+
 typedef struct {
     uint16_t style_start;
     uint16_t style_end;
@@ -183,6 +217,7 @@  static int encode_sample_description(AVCodecContext *avctx)
     int font_names_total_len = 0;
     MovTextContext *s = avctx->priv_data;
     uint8_t buf[30], *p = buf;
+    uint32_t display_flags = 0;
 
     //  0x00, 0x00, 0x00, 0x00, // uint32_t displayFlags
     //  0x01,                   // int8_t horizontal-justification
@@ -241,7 +276,11 @@  static int encode_sample_description(AVCodecContext *avctx)
                      (255 - ((uint32_t)style->back_color >> 24));
     }
 
-    bytestream_put_be32(&p, 0); // displayFlags
+    if (avctx->stream_disposition & AV_DISPOSITION_FORCED)
+        display_flags = DISPLAY_FLAG_SOME_SAMPLES_FORCED |
+            DISPLAY_FLAG_ALL_SAMPLES_FORCED;
+
+    bytestream_put_be32(&p, display_flags); // displayFlags
     bytestream_put_be16(&p, 0x01FF); // horizontal/vertical justification (2x int8_t)
     bytestream_put_be32(&p, back_color);
     bytestream_put_be64(&p, 0); // BoxRecord - 4xint16_t: top, left, bottom, right