[FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Oneric]

Due to a quirk of the ASS format some tags depend on the exact storage
resolution of the video, so tell libass via ass_set_storage_size.

---
ass_set_storage_size exists since libass 0.10.2;
ffmpeg since 5.0 already requires 0.11.0.

This resolution dependences of ASS was already recognised when the
original_size parameter was added, but it actually goes farther than
just the aspect ratio. Conveniently this parameter still has all the
required information to retain rendering after resizing :)

Sample files to show the difference can be found eg here
https://code.videolan.org/videolan/vlc/uploads/b54e0761d0d3f4f79b2947ffb83a3b59/vlc-issue_libass-storage-size.tar.xz

./ffmpeg -i test_1080p.mkv -filter:v ass=./test_1080p.ass tmp_1080.mkv
./ffmpeg -i anamorphic_s720x576_d1024x576.mkv -filter:v ass=./anamorphic_s720x576_d1024x576.ass tmp_anam.mkv

---
 libavfilter/vf_subtitles.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
index 3fc4eeb63d..af6352b315 100644
--- a/libavfilter/vf_subtitles.c
+++ b/libavfilter/vf_subtitles.c
@@ -146,9 +146,14 @@ static int config_input(AVFilterLink *inlink)
     ff_draw_init(&ass->draw, inlink->format, ass->alpha ? FF_DRAW_PROCESS_ALPHA : 0);
 
     ass_set_frame_size  (ass->renderer, inlink->w, inlink->h);
-    if (ass->original_w && ass->original_h)
+    if (ass->original_w && ass->original_h) {
         ass_set_pixel_aspect(ass->renderer, (double)inlink->w / inlink->h /
                              ((double)ass->original_w / ass->original_h));
+        ass_set_storage_size(ass->renderer, ass->original_w, ass->original_h);
+    } else {
+        ass_set_storage_size(ass->renderer, inlink->w, inlink->h);
+    }
+
     if (ass->shaping != -1)
         ass_set_shaper(ass->renderer, ass->shaping);
 
-- 
2.30.2

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Oneric
> Sent: Monday, March 14, 2022 8:07 PM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Oneric <oneric@oneric.de>
> Subject: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size
> to libass
> 
> Due to a quirk of the ASS format some tags depend on the exact storage
> resolution of the video, so tell libass via ass_set_storage_size.
> 
> ---
> ass_set_storage_size exists since libass 0.10.2;
> ffmpeg since 5.0 already requires 0.11.0.
> 
> This resolution dependences of ASS was already recognised when the
> original_size parameter was added, but it actually goes farther than
> just the aspect ratio. Conveniently this parameter still has all the
> required information to retain rendering after resizing :)
> 
> Sample files to show the difference can be found eg here
> https://code.videolan.org/videolan/vlc/uploads/b54e0761d0d3f4f79b2947ffb83
> a3b59/vlc-issue_libass-storage-size.tar.xz
> 
> ./ffmpeg -i test_1080p.mkv -filter:v ass=./test_1080p.ass tmp_1080.mkv
> ./ffmpeg -i anamorphic_s720x576_d1024x576.mkv -filter:v
> ass=./anamorphic_s720x576_d1024x576.ass tmp_anam.mkv
> 
> ---
>  libavfilter/vf_subtitles.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
> index 3fc4eeb63d..af6352b315 100644
> --- a/libavfilter/vf_subtitles.c
> +++ b/libavfilter/vf_subtitles.c
> @@ -146,9 +146,14 @@ static int config_input(AVFilterLink *inlink)
>      ff_draw_init(&ass->draw, inlink->format, ass->alpha ?
> FF_DRAW_PROCESS_ALPHA : 0);
> 
>      ass_set_frame_size  (ass->renderer, inlink->w, inlink->h);
> -    if (ass->original_w && ass->original_h)
> +    if (ass->original_w && ass->original_h) {
>          ass_set_pixel_aspect(ass->renderer, (double)inlink->w / inlink->h
> /
>                               ((double)ass->original_w / ass-
> >original_h));
> +        ass_set_storage_size(ass->renderer, ass->original_w, ass-
> >original_h);
> +    } else {
> +        ass_set_storage_size(ass->renderer, inlink->w, inlink->h);
> +    }
> +
>      if (ass->shaping != -1)
>          ass_set_shaper(ass->renderer, ass->shaping);
> 

Hi,

thanks for the patch!

I've been at the same point some time ago where I wondered why ffmpeg is
not setting this, but then I had found that it is overridden by the call 
to ass_set_pixel_aspect().

ass_set_pixel_aspect() is setting settings.par and if I'm not mistaken,
an existing par setting leads to the storage size setting to be ignored:

https://github.com/libass/libass/blob/5f0e8450f834894b2745238e3d32ff4878710ec8/libass/ass_render.c#L2891-L2903

But perhaps I'm missing something..

softworkz

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Oneric]

On Mon, Mar 14, 2022 at 19:35:36 +0000, Soft Works wrote:
> 
> I've been at the same point some time ago where I wondered why ffmpeg is
> not setting this, but then I had found that it is overridden by the call 
> to ass_set_pixel_aspect().
>
> ass_set_pixel_aspect() is setting settings.par and if I'm not mistaken,
> an existing par setting leads to the storage size setting to be ignored:

It’s not overridden. Only the explicit PAR is currently preferd over the 
implicit derivation from storage and frame size. However as I stated in 
the patch description and the comment:
  “some tags depend on the exact storage resolution of the video”
  “it actually goes farther than just the aspect ratio”

I.e. there's more info in the storage size than just the PAR.
You can also easily test the files I linked to empirically
validate that there is in fact a difference.

> But perhaps I'm missing something..
>
> softworkz
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Oneric
> Sent: Monday, March 14, 2022 8:50 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage
> size to libass
> 
> On Mon, Mar 14, 2022 at 19:35:36 +0000, Soft Works wrote:
> >
> > I've been at the same point some time ago where I wondered why ffmpeg is
> > not setting this, but then I had found that it is overridden by the call
> > to ass_set_pixel_aspect().
> >
> > ass_set_pixel_aspect() is setting settings.par and if I'm not mistaken,
> > an existing par setting leads to the storage size setting to be ignored:
> 
> It’s not overridden. Only the explicit PAR is currently preferd over the
> implicit derivation from storage and frame size. However as I stated in
> the patch description and the comment:
>   “some tags depend on the exact storage resolution of the video”
>   “it actually goes farther than just the aspect ratio”
> 

I found only one other place where storage_h is used (for determining 
blur size) but I didn't find any other usage in the libass source code.
That's what I'm wondering about.

Thanks,
sw


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Oneric]

On Mon, Mar 14, 2022 at 19:57:05 +0000, Soft Works wrote:
> > > ass_set_pixel_aspect() is setting settings.par and if I'm not mistaken,
> > > an existing par setting leads to the storage size setting to be ignored:
> > 
> > It’s not overridden. Only the explicit PAR is currently preferd over the
> > implicit derivation from storage and frame size. However as I stated in
> > the patch description and the comment:
> >   “some tags depend on the exact storage resolution of the video”
> >   “it actually goes farther than just the aspect ratio”
> > 
> 
> I found only one other place where storage_h is used (for determining 
> blur size) but I didn't find any other usage in the libass source code.
> That's what I'm wondering about.

Well, blur is one of the things that depend on it. If you follow the usage 
of the blur scale, you'll see it also plays a role in the projection 
matrix for 3D-transforms (what the provided samples use) and if 
ScaledBorderAndShadow is not set to "yes", it also affects some other 
scaling values.

This unfortunate dependence is a result of how SSA and then ASS 
histoically developed and required to maintain compaitibility with 
existing subtitles.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Oneric
> Sent: Monday, March 14, 2022 9:08 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage
> size to libass
> 
> On Mon, Mar 14, 2022 at 19:57:05 +0000, Soft Works wrote:
> > > > ass_set_pixel_aspect() is setting settings.par and if I'm not
> mistaken,
> > > > an existing par setting leads to the storage size setting to be
> ignored:
> > >
> > > It’s not overridden. Only the explicit PAR is currently preferd over
> the
> > > implicit derivation from storage and frame size. However as I stated
> in
> > > the patch description and the comment:
> > >   “some tags depend on the exact storage resolution of the video”
> > >   “it actually goes farther than just the aspect ratio”
> > >
> >
> > I found only one other place where storage_h is used (for determining
> > blur size) but I didn't find any other usage in the libass source code.
> > That's what I'm wondering about.
> 
> Well, blur is one of the things that depend on it. If you follow the usage
> of the blur scale, you'll see it also plays a role in the projection
> matrix for 3D-transforms (what the provided samples use) and if
> ScaledBorderAndShadow is not set to "yes", it also affects some other
> scaling values.
> 
> This unfortunate dependence is a result of how SSA and then ASS
> histoically developed and required to maintain compaitibility with
> existing subtitles.

Ah, alright, the blur setting goes deeper. Thanks for the explanation.

LGTM, then!

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Oneric]

On Mon, Mar 14, 2022 at 20:06:39 +0100, Oneric wrote:
> Due to a quirk of the ASS format some tags depend on the exact storage
> resolution of the video, so tell libass via ass_set_storage_size.
> [...]

On Mon, Mar 14, 2022 at 20:21:47 +0000, Soft Works wrote:
> [...]
>
> Ah, alright, the blur setting goes deeper. Thanks for the explanation.
> 
> LGTM, then!


ping
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Oneric
> Sent: Tuesday, March 22, 2022 5:28 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass
> storage size to libass
> 
> On Mon, Mar 14, 2022 at 20:06:39 +0100, Oneric wrote:
> > Due to a quirk of the ASS format some tags depend on the exact
> storage
> > resolution of the video, so tell libass via ass_set_storage_size.
> > [...]
> 
> On Mon, Mar 14, 2022 at 20:21:47 +0000, Soft Works wrote:
> > [...]
> >
> > Ah, alright, the blur setting goes deeper. Thanks for the
> explanation.
> >
> > LGTM, then!
> 
> 
> ping

It's not on me. I would have merged it, but I don't have push 
permissions.

softworkz

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[James Almer]

On 3/22/2022 1:42 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Oneric
>> Sent: Tuesday, March 22, 2022 5:28 PM
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass
>> storage size to libass
>>
>> On Mon, Mar 14, 2022 at 20:06:39 +0100, Oneric wrote:
>>> Due to a quirk of the ASS format some tags depend on the exact
>> storage
>>> resolution of the video, so tell libass via ass_set_storage_size.
>>> [...]
>>
>> On Mon, Mar 14, 2022 at 20:21:47 +0000, Soft Works wrote:
>>> [...]
>>>
>>> Ah, alright, the blur setting goes deeper. Thanks for the
>> explanation.
>>>
>>> LGTM, then!
>>
>>
>> ping
> 
> It's not on me. I would have merged it, but I don't have push
> permissions.
> 
> softworkz

Will apply it unless someone is against it.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_subtitles: pass storage size to libass

[Oneric]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On Tue, Mar 22, 2022 at 13:45:20 -0300, James Almer wrote:
> 
> Will apply it unless someone is against it.

Thanks for applying the patch!

In case this fix is eligible for backporting:
It applies nicely at is to the release/5.0 branch and 5.0 also already 
requires a new enough libass for ass_set_storage_size to be always 
available.
For the release/4.[0-4] branches, the attached patch can be used instead.
It applied without problems for me on all the 4.x branches and also built 
and passed FATE with the config I used.

[-- Attachment #2: 0001-avfilter-vf_subtitles-pass-storage-size-to-libass.patch --]
[-- Type: text/x-diff, Size: 1384 bytes --]

From 27b6deafe859eb9bddfb21498a11f2b2b613802b Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.de>
Date: Wed, 23 Mar 2022 20:43:54 +0100
Subject: [PATCH] avfilter/vf_subtitles: pass storage size to libass

Due to a quirk of the ASS format some tags depend on the exact storage
resolution of the video, so tell libass via ass_set_storage_size.
---
 libavfilter/vf_subtitles.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
index de74afa2b7..b57dd80b13 100644
--- a/libavfilter/vf_subtitles.c
+++ b/libavfilter/vf_subtitles.c
@@ -145,9 +145,16 @@ static int config_input(AVFilterLink *inlink)
     ff_draw_init(&ass->draw, inlink->format, ass->alpha ? FF_DRAW_PROCESS_ALPHA : 0);
 
     ass_set_frame_size  (ass->renderer, inlink->w, inlink->h);
-    if (ass->original_w && ass->original_h)
+    if (ass->original_w && ass->original_h) {
         ass_set_aspect_ratio(ass->renderer, (double)inlink->w / inlink->h,
                              (double)ass->original_w / ass->original_h);
+#if LIBASS_VERSION > 0x01010000
+        ass_set_storage_size(ass->renderer, ass->original_w, ass->original_h);
+    } else {
+        ass_set_storage_size(ass->renderer, inlink->w, inlink->h);
+#endif
+    }
+
     if (ass->shaping != -1)
         ass_set_shaper(ass->renderer, ass->shaping);
 
-- 
2.30.2


[-- Attachment #3: 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".