Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: add wrap_unicode option

[Oneric <oneric@oneric.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5019 bytes --]

On Fri, May 19, 2023 at 23:58:59 +0800, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> So CJK can be wrapped automatically.
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavfilter/version.h      |  2 +-
>  libavfilter/vf_subtitles.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/libavfilter/version.h b/libavfilter/version.h
> index ba8a6fdab2..08779130f5 100644
> --- a/libavfilter/version.h
> +++ b/libavfilter/version.h
> @@ -32,7 +32,7 @@
>  #include "version_major.h"
>  
>  #define LIBAVFILTER_VERSION_MINOR   8
> -#define LIBAVFILTER_VERSION_MICRO 100
> +#define LIBAVFILTER_VERSION_MICRO 101
>  
>  
>  #define LIBAVFILTER_VERSION_INT AV_VERSION_INT(LIBAVFILTER_VERSION_MAJOR, \
> diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
> index 82e140e986..d6a68e5cdd 100644
> --- a/libavfilter/vf_subtitles.c
> +++ b/libavfilter/vf_subtitles.c
> @@ -61,6 +61,7 @@ typedef struct AssContext {
>      int original_w, original_h;
>      int shaping;
>      FFDrawContext draw;
> +    int wrap_unicode;
>  } AssContext;
>  
>  #define OFFSET(x) offsetof(AssContext, x)
> @@ -72,6 +73,7 @@ typedef struct AssContext {
>      {"original_size",  "set the size of the original video (used to scale fonts)", OFFSET(original_w), AV_OPT_TYPE_IMAGE_SIZE, {.str = NULL},  0, 0, FLAGS }, \
>      {"fontsdir",       "set the directory containing the fonts to read",           OFFSET(fontsdir),   AV_OPT_TYPE_STRING,     {.str = NULL},  0, 0, FLAGS }, \
>      {"alpha",          "enable processing of alpha channel",                       OFFSET(alpha),      AV_OPT_TYPE_BOOL,       {.i64 = 0   },         0,        1, FLAGS }, \
> +    {"wrap_unicode",   "break lines according to the Unicode Line Breaking Algorithm", OFFSET(wrap_unicode), AV_OPT_TYPE_BOOL, {.i64 = 1   },         0,        1, FLAGS }, \

This feature MUST NOT uncondtionally be enabled by default.
Its documentation clearly warns that it deviates from proper ASS and
is incompatible with VSFilter (the authorative ASS reference
implementation).
Even more, it should imho NEVER be enabled when the input is native ASS.
Existing ASS subtitles and authors of ASS subs (unfortunately) rely on the
existing auto-linebwrapping semantic. For writing systems not working well
with those semantics, authors of native ASS subs already manually break
lines at the appropiate places and enabling this feautre may make the
situation worse, if some lines actually overflow their supposed width.
With the established ASS breaking rules overflowing CJK lines without
spaces will not break on overflow. If they now did, you’d get a
auto-linebreak followed by a manual linebreak one or two characters later.

If subs are not native ASS and only get converted to ASS to be able to
render it with libass (like ffmpeg does), then enabling it is fine.
(Besides converted subs, some programs internally also generate ASS-like
subtitles on-the-fly and pass them to libass, with the resulting subs
never hitting the disk and being distributed, so compatibility is not a
concern. Those programs also may use this feature)

>  
>  /* libass supports a log level ranging from 0 to 7 */
>  static const int ass_libavfilter_log_level_map[] = {
> @@ -329,6 +331,17 @@ static av_cold int init_subtitles(AVFilterContext *ctx)
>          return AVERROR(EINVAL);
>      }
>  
> +    if (ass->wrap_unicode) {
> +#if (LIBASS_VERSION >= 0x01600010)
> +        ret = ass_track_set_feature(ass->track, ASS_FEATURE_WRAP_UNICODE, 1);
> +        if (ret < 0)
> +            av_log(ctx, AV_LOG_WARNING, "libass doesn't build with ASS_FEATURE_WRAP_UNICODE support\n");

Nit: "libass wasn't built with libunibreak support" would imho be clearer.

> +#else
> +        av_log(ctx, AV_LOG_INFO, "libass version %#x doesn't support ASS_FEATURE_WRAP_UNICODE\n",
> +               LIBASS_VERSION);

Nit: This will print the version of libass ffmpeg was built against. This may
be confusing if later on the libass library was upgraded without needing a
rebuild and the new version _does_ support WRAP_UNCIDOE. I’d suggest
something along the lines of
  "ffmpeg was built against libass version %#x not supporting ASS_FEATURE_WRAP_UNICODE"

> +#endif
> +    }
> +
>      /* Open subtitles file */
>      ret = avformat_open_input(&fmt, ass->filename, NULL, NULL);
>      if (ret < 0) {
> -- 


On Fri, May 19, 2023 at 22:14:54 +0800, Lance Wang wrote:
>
> Also, if the version is right, then why to check the return?
> +#if (LIBASS_VERSION >= 0x01600010)
> +        ass_track_set_feature(ass->track, ASS_FEATURE_WRAP_UNICODE, 1);
> +#endif

libass API provides WRAP_UNICODE since 0x01600010,
but at runtime the feature is only available if libass
was built against its optional dependency libunibreak.
If not attempting to enable the feature will have
no effect and ass_track_set_feature returns -1.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".