| Message ID | 20220314190638.24816-1-oneric@oneric.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/vf_subtitles: pass storage size to libass | expand |
| 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 |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
> -----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
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
> -----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
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.
> -----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!
> -----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
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.
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.
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);