Second shear computed differently than VSFilter

[GoogleCodeExporter]

Comparing this rendering of this line:

{\fnTahoma\c&H000000&\fs100\an7\fay0.115\pos(240,250)}——————–>{\fa
y0.0}——>

To this line:

{\fnTahoma\c&H000000&\fs100\an7\fay0.0\pos(240,250)}——————–>{\fay0
.0}——>

The second arrow is in the same place for both lines when using VSFilter, but 
the origin of the second shear is different when using libass.

Original issue reported on code.google.com by BwackNi...@gmail.com on 31 Dec 2012 at 5:42

[GoogleCodeExporter]

This is quite a WTF. VSFilter seems to reset the baseline for the second arrow 
if the shear changes. But if the shear is _exactly_ the same, it doesn't reset 
the baseline.

For example this lets the second arrow start from Y position 250:

{\fnTahoma\c&H000000&\fs100\an7\fay0.715\pos(240,250)}——————–>{\fa
y0.716}——>

While this lets it start where the first arrow ends:

{\fnTahoma\c&H000000&\fs100\an7\fay0.715\pos(240,250)}——————–>{\fa
y0.715}——>

libass just always starts the second arrow where the first arrow ended. This 
happens even if the Y shear is unchanged, but you change the X shear with \fax. 
There might be more tricky behavior to it.

Other than the WTF, the basic problem is that changing shear somehow resets the 
text layout position. Does anyone know whether this is easily fixable?

Original comment by nfxjfg@googlemail.com on 23 Mar 2013 at 11:27

[GoogleCodeExporter]

The second arrow also starts at the end of the first arrow if the first shear 
is 0.

The shear also has no impact on the start of the next line as shown because 
this line:

{\fnTahoma\c&H000000&\fs100\an7\fay0.115\pos(240,250)}——————–>\N��
�—>

and this line:

{\fnTahoma\c&H000000&\fs100\an7\fay0.0\pos(240,250)}——————–>\N—�
��>

have the second arrow in the same place, while libass does not. After some 
playing around, it is really that after every change in \fay or a newline, the 
vertical offset resets to 0.

I believe the fix would need to be somewhere around line 1876 of ass_render.c:

        // Preliminary layout (for line wrapping)

You could store the previous pen.y value and reset pen.y to that every time the 
\fay changes or a newline is found.

Original comment by BwackNi...@gmail.com on 27 Mar 2013 at 7:21

[GoogleCodeExporter]

Doesn't seem to be that easy. If you have a patch that works, please post.

Original comment by nfxjfg@googlemail.com on 27 Mar 2013 at 10:52

[GoogleCodeExporter]

This patch still needs to be rebased against git, I only used 0.10.0 when 
making it. It's simple enough and seems to fix all the cases mentioned here. It 
was just a quick change, but could use some cleanup, like how the addition to 
info->cluster_advance.y is calculated twice.

Original comment by BwackNi...@gmail.com on 28 Mar 2013 at 10:11

[GoogleCodeExporter]

Nope, that only seems to fix the single-line case and introduces multi-line 
issues.

Original comment by BwackNi...@gmail.com on 28 Mar 2013 at 11:43

[GoogleCodeExporter]

Yes, and I don't really know how that stuff works. I can only encourage you to 
try to perfect your patch, or wait until Greg takes a look. (Personally I'm a 
bit too afraid of breaking something, and I don't really know how the 
interaction with the shaper works. Other than that I don't care enough about 
fixing this, unless there's really a significant number of subtitles that break 
due to this.)

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

[GoogleCodeExporter]

Have you checked whether the baseline also changes/resets if another layout 
property is changed? VSFilter's rendering works in terms of text runs with the 
same properties. But there are some serious bugs. This leads to really funny 
behavior.

For instance, if a line is split up into multiple runs by VSFilter (maybe 
because you change the font), the font is scaled with \fsc{x,y} and opaque box 
is used for the outline, the box might be sized in a funny way, depending on 
where the line was split up. The behavior we're seeing with vertical shearing 
is probably also related to this.

Generally, I don't think it's worth the effort to emulate something like this 
entirely. There's a lot of complexity involved and it's almost impossible to 
get it right. A solution that achieves compatible rendering in most cases with 
little code complexity is preferable to an intricate, delicate solution that is 
entirely compatible. If you go for the 100% compatible route, you'll just 
become insane at some point or at least the code will become convoluted and 
unmaintainable. :)

I haven't really looked at the patch yet, I'll get back to it after easter.

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

[GoogleCodeExporter]

Alright, for when you take a look at the patch, here's a fresh one that solves 
all the \fay issues mentioned here. I think it is a simple enough patch that 
maintainability shouldn't be an issue.

Beyond simple testcases, the real world test that inspired this bug report was:
{\fnSwis721 Lt 
BT\c&H8B947C&\fs20\an7\fay0.115\fax0.05\fry3\blur0.8\b1\move(204,244,204,348,63,
5234)}Apparently the final ep of GUNVARREL has been leaked 
online.\N\N\N\N\N{\fscy60}\N{\fscy100} Seriously? They never aired the final 
episode, right?\N\N\N\N\N{\fscy70}\N{\fscy100}\N\h\h\h\h\h\h\h {\fay0.11}? if 
true sauce pls\N\N\N\N\N{\fscy70}\N{\fscy100\fay0.105} I've known about the 
final episode for a year now actually\N\N\N\N\N\N\h{\fay0.108\t(\c&H777B6B&)} I 
never expected a story development like that at Episode 3. I'm glued to the 
screen now LOL\N\N\N\N\N\N\h So someone on the staff leaked it?

(This line still has an issue because \N is not scaled by \fscy in libass, but 
it is in vsfilter. It looks correct after fixing that issue as well.)

Original comment by BwackNi...@gmail.com on 29 Mar 2013 at 8:30

Attachments:

• issue_80.patch

[GoogleCodeExporter]

I think this patch doesn't look too bad.

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

[GoogleCodeExporter]

I realized a bit of the checks were redundant, so here's a cleaner patch.

As a sidenote, shouldn't "glyphs[i].linebreak" be "info->linebreak" instead to 
avoid breaking rtl?

Original comment by BwackNi...@gmail.com on 2 Apr 2013 at 5:20

Attachments:

• issue_80_2.patch

[GoogleCodeExporter]

Is the (glyphs + cmap[0])->fay access always guaranteed to work? I see nothing 
that guarantees this.

Original comment by nfxjfg@googlemail.com on 3 Apr 2013 at 10:21

[GoogleCodeExporter]

It would only fail if text_info->length = 0. last_fay can just be set to 0 
instead because it will only matter if pen.x != 0, and pen.x = 0 for the first 
iteration while last_fay is set before the next iteration.

Original comment by BwackNi...@gmail.com on 3 Apr 2013 at 11:03

[GoogleCodeExporter]

LGTM, this is reasonably clean, and hopefully won't break anything. :)

Original comment by g...@chown.ath.cx on 12 Apr 2013 at 4:32

[GoogleCodeExporter]

I wanted to push issue_80_2.patch, but when I tested it, I found that it didn't 
fix the issue for me.

Maybe it's because I'm not using harfbuzz? I can't tell why. Frankly, I don't 
get this part of the layouting code at all.

Original comment by nfxjfg@googlemail.com on 12 Apr 2013 at 4:55

[GoogleCodeExporter]

Indeed, issue_80.patch works but issue_80_2.patch does not work. I believe this 
is because it resets last_pen_x on every glyph instead of only newlines and 
text run boundaries.

I can also confirm that VSFilter resets the baseline on any text run boundary, 
not just on \fay changes. I think it's pretty clear what's happening here: it 
just never changes the baseline at all and applies shear to whole runs.

Original comment by chortos@inbox.lv on 22 Apr 2013 at 9:45

[GoogleCodeExporter]

Sorry, I was a little overzealous when I was cleaning up the patch, here's one 
that works. (Not attached because it says "Issue attachment storage quota 
exceeded.")

https://docs.google.com/file/d/0B4qGSo7_ixyFVjlHYnk1QXZxTHM/edit?usp=sharing

Original comment by BwackNi...@gmail.com on 22 Apr 2013 at 10:32

[GoogleCodeExporter]

Sorry, I didn't notice that you posted a new patch. So this issue got lost for 
half a year, until someone pointed it out to me just today.

The patch looks good and seems to work. I'm posting your exact patch again, so 
everyone can look at it without going through google docs.

I'll push the patch in one day if nobody is against it.

Original comment by nfxjfg@googlemail.com on 23 Oct 2013 at 10:50

Attachments:

• issue_80_3.patch

[GoogleCodeExporter]

Thanks, I'm glad to see this moving forward. :)

Original comment by BwackNi...@gmail.com on 23 Oct 2013 at 11:26

[GoogleCodeExporter]

Pushed.

Original comment by nfxjfg@googlemail.com on 24 Oct 2013 at 11:08

• Changed state: Done