Closed GoogleCodeExporter closed 8 years ago
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
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
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
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
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
Original comment by g...@chown.ath.cx
on 27 Sep 2011 at 12:25
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
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
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
> 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
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
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
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:
Original comment by g...@chown.ath.cx
on 10 Mar 2013 at 3:43
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
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
>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
> 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
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:
>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
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
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:
> 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
Thanks. Then I'll push this in 20 hours or so.
Original comment by nfxjfg@googlemail.com
on 18 Mar 2013 at 3:58
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
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:
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
Anyway, API cleanup is a separate issue from this one.
Original comment by g...@chown.ath.cx
on 20 Mar 2013 at 12:23
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
Pushed. (A bit later than just 20 hours, oh well.)
Original comment by nfxjfg@googlemail.com
on 29 Mar 2013 at 10:12
Original issue reported on code.google.com by
erapple...@gmail.com
on 26 Aug 2009 at 5:32Attachments: