Scaling of \blur tag is different from VSFilter

[GoogleCodeExporter]

Source: [Mazui] Suzumiya Haruhi no Yuuutsu (2009) - 21 [6CF2660F].mkv

Script and fonts are included in attachment.

Style: Default,Myriad Pro
SemiCond,22,&H00F8EAE9,&H008FA4A3,&H6431080D,&H80000000,-1,0,0,0,100,100,0,0,1,1
.2,0.6,2,8,8,7,0

Style: cast,I hate Comic
Sans,14,&H00333633,&H000000FF,&H00000000,&H00000000,0,0,0,0,100,100,1,0,1,0,0,2,
10,10,10,1

Dialogue:
0,0:03:39.56,0:03:43.69,Default,,0000,0000,0000,,{\fad(500,500)\fs25\fsp-1\4c&H2
22222&\fscx95\shad1.5\blur1\b1\4a&H00&\bord0\1c&HF8FCFD&\pos(218,50)}The
Sighs of Suzumiya Haruhi II

Dialogue:
0,0:06:20.23,0:06:22.23,cast,,0000,0000,0000,,{\frz358.75\fs20\blur2\pos(372,41)
}Cast

Dialogue:
0,0:06:20.23,0:06:22.23,cast,,0000,0000,0000,,{\an4\frz85.218\blur2\pos(287,159)
}Asahina
Mikuru - the Battle \NWaitress from the future

Dialogue:
0,0:06:20.23,0:06:22.23,cast,,0000,0000,0000,,{\frz85.218\blur2\pos(247,137)}Koi
zumi
Itsuki - the Esper Youth

Dialogue:
0,0:06:20.23,0:06:22.23,cast,,0000,0000,0000,,{\frz85.218\blur2\pos(206,121)}Nag
ato
Yuki - the Evil Alien

Dialogue:
0,0:06:20.23,0:06:22.23,cast,,0000,0000,0000,,{\frz85.218\blur2\pos(157,148)}Ext
ras
- everyone else{Norg: passers by.}

Original issue reported on code.google.com by erapple...@gmail.com on 26 Aug 2009 at 5:32

Attachments:

• mazui-haruhi21-libass.png
• mazui-haruhi21-vsfilter.png
• mazui-haruhi21.tar.gz

[GoogleCodeExporter]

I'll probably add a toggle to configure whether \blur should be scaled like 
borders.

Original comment by g...@chown.ath.cx on 6 Sep 2009 at 10:31

• Changed title: Scaling of \blur tag is different

[GoogleCodeExporter]

Just an idea (not sure of the current interface allows it): do not scale from 
script 
size to video size, but scale from video size to subtitle raster size.

Example:
Script: 720x480 (PlayResX/Y)
Video: 1280x720 (encoded video size)
Playback: 1920x1280 (resolution at which the subtitles are rendered)

What you do: scale like you are going from 720x480 to 1920x1080
What you should do: scale like you are going from 1280x720 to 1920x1080

Original comment by labr...@sogetthis.com on 6 Feb 2010 at 12:42

[GoogleCodeExporter]

Unfortunately libass does not know about the video's resolution (and shouldn't 
either).

Original comment by g...@chown.ath.cx on 7 Feb 2010 at 3:52

[GoogleCodeExporter]

I disagree that it shouldn't since you need to know it to fix issues like this 
one. To be very pedantic there's 
subtler issues too. For example, with ScaledBorderAndShadow toggled off.

If the script, video and playback sizes are very different and 
ScaledBorderAndShadow is disabled, the results are 
going to be bad. With the same example above, maybe the style was done thinking 
about 1280x720. So it sets a border 
of 2, thinking "it looks very good for this resolution". But then you go to 
1920x1080 and it's too thin.

The most accurate solution is: if ScaledBorderAndShadow is on, do as you do now 
(scale from "script" to "playback"). 
If it's off, scale from "video" to "playback" (instead of not scaling at all).

This is a different issue, but it also would require knowing the video size. It 
depends on how faithful you want to 
be (because libass already tries to do pixel aspect ratio correction for 
example, which falls into the same category).

Original comment by labr...@sogetthis.com on 7 Feb 2010 at 4:13

[GoogleCodeExporter]

The goal I set for libass is to achieve similar rendering, independent of 
rendering 
resolution (or video resolution for that matter) and that's what you currently 
see. 
PAR might be related to this, but it's not quite the same thing.

Of course ScaledBorderAndShadow=false subverts all scaling, but that's exactly 
what 
it is supposed to do, as stupid as this may be.

Personally I'd like to stop implementing all the nonsensical stuff found in 
VSFilter, 
if possible. Almost all bug reports I get nowadays (a lot is only reported via 
IRC) 
are due to VSFilter crazyness.

Original comment by g...@chown.ath.cx on 7 Feb 2010 at 12:59

[GoogleCodeExporter]

Original comment by g...@chown.ath.cx on 27 Sep 2011 at 12:25

• Changed title: Scaling of \blur tag is different from VSFilter

[GoogleCodeExporter]

Here's a patch (split off issue #78):
    http://code.google.com/r/astiob-libass/source/detail?r=410802226b3b2627637c7c0b256181c18b508fe5

The only downside I can see is that until video players adapt to this change, 
the following scenario will become broken:
* the script resolution is the same as the storage resolution, and
* the video player renders subtitles at the display resolution, and
* the user asks the player to resize the video, and
* the script uses \blur.
If it is important to retain correct default behaviour in this situation (while 
uncorrecting default behaviour in other situations), I can adjust the patch to 
make the blur and border scale defaults different.

Original comment by chortos@inbox.lv on 2 Feb 2013 at 5:11

[GoogleCodeExporter]

Looks mostly ok to me.

One issue: why does ass_set_storage_size not allow resetting the size to 0/0? 
This could be used to restore the default values.

Also, from what I can see, this does retain the previous behavior if 
ass_set_storage_size is not called, doesn't it?

Original comment by nfxjfg@googlemail.com on 3 Feb 2013 at 11:28

[GoogleCodeExporter]

Oh, I forgot another condition:
* and ScaledBorderAndShadow=yes.

Basically, the new storage_height defaults to the display resolution (excluding 
margins) if ass_set_storage_size is not called, which makes blur_scale default 
to 1. But before the patch, border_scale was used instead of blur_scale, and 
border_scale may not be equal to 1 at this point.

And about disallowing resetting the size to 0/0... I had trouble remembering 
why I did it myself. I guess I wasn't sure what to reset storage_aspect to. 
storage_aspect in general is troublesome because sensibly, ass_set_frame_size 
and ass_set_storage_size should be callable in any order with the same effects, 
but both try to set storage_aspect.

After some thinking now, I think I should just remove all storage_aspect 
assignments from both functions and instead add a conditional to 
ass_start_frame (the only user of storage_aspect) to check if storage_aspect 
has been initialized (by ass_set_aspect_ratio). In fact, it would make sense to 
do the same for the display aspect as well: currently, if for any reason you 
make two calls to ass_set_frame_size and no call to ass_set_aspect_ratio, the 
display aspect is determined by the first of the two calls instead of the 
second one.

Original comment by chortos@inbox.lv on 3 Feb 2013 at 3:21

[GoogleCodeExporter]

> But before the patch, border_scale was used instead of blur_scale, and 
border_scale may not be equal to 1 at this point.

Oh, right. Overlooked that.

Original comment by nfxjfg@googlemail.com on 3 Feb 2013 at 6:45

[GoogleCodeExporter]

This should be fixed now if the player makes use of ass_set_storage_size.

Original comment by g...@chown.ath.cx on 5 Mar 2013 at 3:52

• Changed state: Fixed

[GoogleCodeExporter]

Trying to implement this, I found a big problem: both ass_set_storage_size() 
and ass_set_aspect_ratio() set priv->settings.storage_aspect. Also, change 
detection for ass_set_storage_size() only works half: even if storage_aspect 
has been overwritten, the function won't reset it.

You could probably get by with calling ass_set_aspect_ratio first and then 
ass_set_storage_size. However, you can call ass_set_aspect_ratio() never again 
without breaking something, because it will see the different storage_aspect, 
overwrite the value, and trigger reconfiguration. Unless after this you 
deliberately call ass_set_storage_size() twice with different arguments to 
break the change detection.

This is really fragile, and breaks a typical way to use libass: call all setup 
functions before the next ass_render_frame() call, and rely on the change 
detection so that the cache isn't invalidated every time.

How to fix? The central evil is that both try to set storage aspect. Maybe 
remove the storage aspect parameter from ass_set_aspect_ratio(), and force the 
user to use ass_set_storage_size()? Maybe allow passing 0 for width and height, 
so that you can get the old behavior?

Original comment by nfxjfg@googlemail.com on 9 Mar 2013 at 9:30

[GoogleCodeExporter]

Actually, I think the solution is not letting ass_set_storage_size() set the 
storage_aspect. It just makes no sense, and in my code I'm always forcing the 
SAR to 1.0.

Also, about storage aspect ratio: you don't want to force the "real" aspect 
ratio. If you have anamorphic material (where the SAR is something other than 
1), you still might render it to a destination with square pixels. And you want 
it to look exactly the same as with a rendering method that renders directly 
into the image. So you have to let the application choose the aspect ratio for 
the font.

(Also, PAR and SAR are actually pixel aspect ratios - just saying to avoid 
further confusion.)

Patch attached.

PS: isn't the SAR completely useless? libass uses font_scale_x=DAR/SAR, and 
niether DAR or SAR seem to be used anywhere else. So one of these values is 
redundant.

Original comment by nfxjfg@googlemail.com on 9 Mar 2013 at 9:59

Attachments:

• p.patch

[GoogleCodeExporter]

Original comment by g...@chown.ath.cx on 10 Mar 2013 at 3:43

• Changed state: Accepted

[GoogleCodeExporter]

I will push the attached patch in 2 days, unless someone has another suggestion 
how this should be handled.

Original comment by nfxjfg@googlemail.com on 17 Mar 2013 at 11:40

[GoogleCodeExporter]

Sorry, I really seem to have gotten sloppy recently. Here's the reply I 
should've written days ago.

We touched upon this issue before: see comments 8 and 9. I think the complete 
solution is to remove not just the storage aspect assignment in 
ass_set_storage_size but also the storage and display aspect assignments in 
ass_set_frame_size. What they are logically intended to do is provide fallback 
values when ass_set_aspect_ratio is never called; this logic should instead be 
moved to where the aspect ratios are actually used, namely ass_start_frame.

> (Also, PAR and SAR are actually pixel aspect ratios - just saying to avoid 
further confusion.)

Are they? The ass_set_*_size functions initialize them to image_width / 
image_height, which is the image aspect ratio given square pixels.

> PS: isn't the SAR completely useless? libass uses font_scale_x=DAR/SAR, and 
niether DAR or SAR seem to be used anywhere else. So one of these values is 
redundant.

It sounds like we could avoid quite a bit of confusion by just replacing the 
two aspect ratio values with a single user-settable font scale value.

Original comment by chortos@inbox.lv on 18 Mar 2013 at 12:03

[GoogleCodeExporter]

>What they are logically intended to do is provide fallback values when 
ass_set_aspect_ratio is never called; this logic should instead be moved to 
where the aspect ratios are actually used, namely ass_start_frame.

That's fine. Should I post an updated patch? ass_set_aspect_ratio() would 
calculate a PAR (see below), and ass_start_frame() would calculate a fallback 
if ass_set_aspect_ratio() hasn't been called by the user.

>> (Also, PAR and SAR are actually pixel aspect ratios - just saying to avoid 
further confusion.)

>Are they? The ass_set_*_size functions initialize them to image_width / 
image_height, which is the image aspect ratio given square pixels.

OK, they are not necessarily pixel aspect ratios. But if you use 
ass_set_aspect_ratio, they can be. Nothing mandates that they are proper 
aspects, and various mplayer clones use them as pixel aspect ratios.

>It sounds like we could avoid quite a bit of confusion by just replacing the 
two aspect ratio values with a single user-settable font scale value.

Sure. I was about to do this, but then I realized that ass_set_aspect_ratio() 
basically just sets a par, and an interface change is not really needed. What 
gets in the way is the other functions overwriting the user-set aspect.

Original comment by nfxjfg@googlemail.com on 18 Mar 2013 at 1:34

[GoogleCodeExporter]

> Should I post an updated patch? ass_set_aspect_ratio() would calculate a PAR 
(see below), and ass_start_frame() would calculate a fallback if 
ass_set_aspect_ratio() hasn't been called by the user.

I'll say yes just in case, but the description sounds exactly right.

> Nothing mandates that they are proper aspects, and various mplayer clones use 
them as pixel aspect ratios.

Just a thought: can mplayer clones perhaps be changed to actually use the 
display/rendering and storage dimensions? Maybe even without calling 
ass_set_aspect_ratio at all. Of course, this isn't necessary at all, but it 
could let the code make a bit more sense.

Original comment by chortos@inbox.lv on 18 Mar 2013 at 1:58

[GoogleCodeExporter]

OK here's a patch. Setting the storage size won't override 
ass_set_aspect_ratio() anymore. If ass_set_aspect_ratio() is used, 
storage_width is effectively ignored (only storage_height is needed to 
calculate the blur size). Internally, only the PAR is left, and calculations 
are moved to ass_start_frame(). I also made the doxygen comment more precise.

>can mplayer clones perhaps be changed to actually use the display/rendering 
and storage dimensions? Maybe even without calling ass_set_aspect_ratio at all.

I think letting the user set the aspect ratio is a good thing. mplayer actually 
uses this to deal with the differences between subtitle rendering methods that 
need aspect corrections and those that don't, or for forcing an output aspect 
ratio (like for the -monitorpixelaspect option). I'm not really sure what 
MPlayer does, but I think there are 3 dimensions involved - original video 
size, display size, and size of the thing you render to. The mplayer fork I'm 
working on (mpv) does similar things, though I keep them all as PARs and then 
set it as ass_set_aspect_ratio(renderer, par, 1).

Summary: it's easier for the user to set a PAR, but ass_set_aspect_ratio() 
taking two aspect values probably confuses everyone and is not needed.

Original comment by nfxjfg@googlemail.com on 18 Mar 2013 at 3:08

Attachments:

• issue6.patch

[GoogleCodeExporter]

>there are 3 dimensions involved - original video size, display size, and size 
of the thing you render to

Correction: only if it makes use of ass_set_storage_size(). Like I explained in 
comment 13, the frame size (as set by ass_set_frame_size()) can include margins 
(like vo_gl in mplayer), it can be a screen with PAR=1 (like vo_gl in mplayer), 
or it can be an image (like vf_ass in mplayer, needed for vo_xv).

Further complication: in mplayer2 you can disable the "vsfilter aspect ratio". 
VSFilter traditionally renders always into the image, so you need to apply the 
storage aspect even if you render to the screen. But sometimes you don't want 
that. Big mess.

Original comment by nfxjfg@googlemail.com on 18 Mar 2013 at 3:14

[GoogleCodeExporter]

The code looks fine to me. I've noticed a couple of typos in the comment (if 
the PAR *is* 0 or if it *has* never been set), and you should change 
ass_set_storage_size's comment too.

Original comment by chortos@inbox.lv on 18 Mar 2013 at 3:24

[GoogleCodeExporter]

New patch. I also changed it so that you can call ass_set_storage_size with 0 
to reset it. Sorry for the typos, it's a bit late.

Original comment by nfxjfg@googlemail.com on 18 Mar 2013 at 3:41

Attachments:

• issue6_2.patch

[GoogleCodeExporter]

> I also changed it so that you can call ass_set_storage_size with 0 to reset 
it.
Ah, right. I forgot about this.

The new patch looks good!

Original comment by chortos@inbox.lv on 18 Mar 2013 at 3:51

[GoogleCodeExporter]

Thanks. Then I'll push this in 20 hours or so.

Original comment by nfxjfg@googlemail.com on 18 Mar 2013 at 3:58

[GoogleCodeExporter]

I think at this point we should definitely simplify ass_set_aspect_ratio and 
remove the SAR parameter. This will be less confusing.

Original comment by g...@chown.ath.cx on 19 Mar 2013 at 1:04

[GoogleCodeExporter]

Not too fond of breaking the API, so here's a patch that deprecates 
ass_set_aspect_ratio() and introduces ass_set_pixel_aspect().

Regarding API changes: we recently broke the ABI, but an API change is a bit 
more radical. We can delay the removal of ass_set_aspect_ratio until there are 
actually radical API changes.

Also, I made the patches with git format-patch, so you can check the commit 
messages.

Original comment by nfxjfg@googlemail.com on 20 Mar 2013 at 12:12

Attachments:

• 0001-Don-t-overwrite-user-defined-aspect-ratio-settings.patch
• 0002-Add-ass_set_pixel_aspect-deprecate-ass_set_aspect_ra.patch

[GoogleCodeExporter]

Well, the API really needs a cleanup, and it has been stable since 0.9.7 
(released over 3 years ago), so it's not like we'd be breaking it all the time. 
There is so much cruft that needs to be cleaned up, e.g. ass_set_fonts and 
friends related to font management. ASS_Library should also go away IMO and 
needs to be incorporated into ASS_Renderer. What do you think?

Original comment by g...@chown.ath.cx on 20 Mar 2013 at 12:20

[GoogleCodeExporter]

Anyway, API cleanup is a separate issue from this one.

Original comment by g...@chown.ath.cx on 20 Mar 2013 at 12:23

[GoogleCodeExporter]

Can you make a new issue about the ASS_Library cleanup idea? Then I'll comment.

Original comment by nfxjfg@googlemail.com on 20 Mar 2013 at 12:26

[GoogleCodeExporter]

Pushed. (A bit later than just 20 hours, oh well.)

Original comment by nfxjfg@googlemail.com on 29 Mar 2013 at 10:12

• Changed state: Fixed