Also, I think it makes more sense to run bidi on the whole event, rather than on each explicitly-delimited line (as we do currently), no?

The question is, given:

[English][Arabic]\N[Arabic][English]

is it more likely to mean

1. English with embedded Arabic and manually wrapped lines? or
2. two distinct texts, the first English-with-embedded-Arabic and the second Arabic-with-embedded-English?

Because our current code (with Encoding=-1, otherwise there’s nothing to discuss) does 2.

In fact, Unicode seems to mandate the latter behaviour, saying the text must first be split on paragraph separators, among which it lists the line feed, and then each paragraph’s base direction must be separately autodetected. This is not what FriBidi does though: it does split paragraphs on line feeds, but it runs direction detection only once for the whole input buffer. And I still think that makes more sense for our use case.

Pushed several additional fixes for bidi and shaping (and fixed the Travis CI build failure). And renamed the feature to ASS_FEATURE_WHOLE_TEXT.

This improves both our compatibility with VSFilter when the new feature is off (I think we should be identical now, barring shaping differences between Uniscribe and HarfBuzz) and our rendering when the new feature is on.

Maybe call it ASS_FEATURE_WHOLE_TEXT_BIDI for better clarity?

Two reasons I didn’t:

• It affects shaping as well. Our internal code calls bidi and shaping together “shaping”, but that isn’t right either. I haven’t been able to come up with a simple term that covers both. Whole text typography? (Not really.) Whole text processing?

• Perhaps the same feature could be extended in the future for more run un-compatibility-fixes, e. g. we could try to combine bitmaps (especially BorderStyle 3) for whole lines instead of per run. Or would this be too unstable of an API?

Since the last push, this PR additionally affects baseline reset with \fay:

Without WHOLE_TEXT, follow VSFilter and reset the baseline for each run.

With WHOLE_TEXT, try to shear each line as a whole.
There are still issues though:

• \fscx0 glyphs are skipped and do not contribute any shear;
• applying shear to each glyph and then scaling it together with
  the shear makes little sense for whole lines to begin with.

Perhaps with WHOLE_TEXT we should apply shear after scaling: this would seem to make more sense. But this would be a bigger change. On the other hand, years ago, libass actually used to do this, before it was changed for VSFilter compatibility.

As for the default setting:

How about we make it default to whole-line bidi for Encoding=-1 and VSFilter-bidi for all other Encoding values? -1 is our existing bidi extension that changes base paragraph direction.

Unfortunately, being a left-to-right user myself, I don’t know (and haven’t investigated yet) whether this compromise would help in practice. But it sounds like it might.

Two reasons I didn’t:

* Perhaps the same feature could be extended in the future for more run un-compatibility-fixes, e. g. we could try to combine bitmaps (especially BorderStyle 3) for whole lines instead of per run. Or would this be too unstable of an API?

I think I agree, this look bad #148 also here https://web.archive.org/web/20160120113701/https://code.google.com/p/libass/issues/detail?id=114

it can be happen even in English

vs vsfilter

As for the default setting:

How about we make it default to whole-line bidi for Encoding=-1 and VSFilter-bidi for all other Encoding values? -1 is our existing bidi extension that changes base paragraph direction.

Unfortunately, being a left-to-right user myself, I don’t know (and haven’t investigated yet) whether this compromise would help in practice. But it sounds like it might.

seems fine, I really hope to fix those soon so I can let vsfilter RIP and only use libass

using assrender https://github.com/pinterf/assrender , both avs+ and assrender should work in Linux nowadays (I did use windows) and since its easy to build and test using avspmod now (at least for me) I did some tests

using the sample from #71

LoadcPlugin("D:\assrender-master\bin\Release_Win32\assrender runs_bidi_shaping.dll")
ColorBars
converttoyv12
assrender("D:\AvisynthRepository\script.ass")

it seems work now

BUT #374 now look like this

Thanks! I can confirm that the sample in #374 is broken and I’ve tracked down the bug. The specific sample in #374, with the explicit line break, will now be fixed. But this has highlighted a problem.

We run bidi, then wrap lines.

But each line is its own run and needs its own bidi.

I don’t know how to fix this reliably.

For example: take the sample from #374 and replace the line break by a space. Use a font large enough that the automatic line wrapping breaks the line in the same place as before. VSFilter will show the same result that is seen in #374, but libass—both with and without this PR—will show the output in the screenshot above.

I’ve pushed a new version:

• The first commit now enables whole-text processing for events with Encoding −1 regardless of the state of the new ASS_Feature. This preserves our current whole-line bidi, and the later commits further improve bidi and shaping.

• I’ve reverted my decision from the first two comments on this PR and removed the “autodetect base direction for whole event, not per line” commit. Several reasons for this:

  • I had misunderstood FriBidi code: fribidi_get_par_embedding_levels really expects only a single paragraph as input and does not internally split it, but rather stops looking for explicit bidi controls after it reaches the first paragraph terminator (including '\n'). So my commit broke explicit bidi controls beyond the first line break.
  • WebVTT demands independent direction autodetection for each line within a multiline event. (See A text alignment, in particular Example 18.)
  • I checked macOS’s stock text editor (which generally does typography well), and it autodetects direction for each LF-delimited line separately, too. (Although my macOS is far from latest.)
  • This change also “broke” Bidi bracket matching is VSFilter-incompatibile #374, which is what thankfully made me notice it, but there’s a deeper reason why it really broke; see previous comments and below.

• I’ve accordingly reworked the “remember resolved base direction” commit to account for the fact that each paragraph has its own direction. (Note that direction autodetection can only happen when shaper->whole_text, as it now enforces whole-text processing.)

What remains is to somehow re-bidi runs that line wrapping splits into multiple parts.

An obvious, dumb approach is to simply rerun bidi and shaping on such runs after line wrapping. But it may be possible that this changes their widths and invalidates the line wrapping itself.

it's seems fine now, I will do more test later

Thanks for the (initial) review! I’ve rebased on master and thrown out the duplicate commits. In retrospect, I should’ve done this before deleting the fay-fscxy branch to reduce the potential for human error during rebasing :-)

But why do we want leading operators again?

We have 32 instances of leading operators, in 21 distinct consecutive-line blocks, as per this regex:

^\s*([|^%>]|/[^/*]|\+[^+]|-[^-]|&[& ]|< )

including 11 operators in 3 line blocks in ass_shaper.c before this PR, none of which are modified in this PR if I’m not mistaken.

We have 165 instances of trailing operators, in 88 distinct consecutive-line blocks, as per this analogous regex:

([|^%<]|[^/*]/|[^+]\+|[^-]-|[& ]&| >)\s*$

including 11 operators in 5 line blocks in ass_shaper.c before this PR, none of which are modified in this PR if I’m not mistaken; but neighbouring lines are modified and, currently, kept consistent with trailing operators.

Were there any blockers/todos left for this? (Apart from it merge-conflicting with the also pending #545)

Todos yes: rerun bidi after line wrapping somehow. But I don’t think it should be a blocker, as the current PR still provides a strict improvement in VSFilter compatibility and hopefully covers most of the real-world cases.

Plus bikesheds: about the feature name and about the feature mechanism itself.

Plus bikesheds: about the feature name and about the feature mechanism itself.

iirc none of us could come up with a better name than WHOLE_TEXT and imho redesigning the internal feature handling is orthogonal to adding WHOLE_TEXT. LGTM.

Quick summary of our naming bikeshed brainstorm a while ago on IRC:

• What I was really going for here is “text layout without the quirks caused by VSFilter’s run splitting”, or in other words, “text layout that you can make sense of without knowing about VSFilter’s (or libass’s) run splitting”. We concluded that the keyword is “layout”.

  This is why it covers bidi, shaping, \fsp and even shear, but not karaoke filling or opaque borders.

  Now that I’m writing this, I guess counting shaping as layout is a bit odd. It does definitely affect layout though.

  And I might’ve liked to add karaoke to this in the future, but if we settle on this naming, it’ll just have to be separate.

• MrSmile has proposed SEAMLESS_TEXT_LAYOUT. I’ve grown to like it, except that “seamless” smells funny. What do native speakers think?

• MrSmile has proposed SEAMLESS_TEXT_LAYOUT. I’ve grown to like it, except that “seamless” smells funny. What do native speakers think?

In absence of any native speaker feedback on “semaless”, does WHOLE_TEXT_LAYOUT smell less funny and facilitate a merge?

hi, sorry for my late (I've been busy for over a month) I have no problem with SEAMLESS_TEXT_LAYOUT, if it about "funny" then I think it already is in ".ass"

Rebased on top of current master with the new ASS_Feature internals, and renamed to WHOLE_TEXT_LAYOUT. If anyone wants to give this a final look-over or test-run just in case I messed something up during the rebase, please do so. Barring objections, I think I’ll merge this in a couple of days.

The range-diff of the rebase seems perfectly fine to me and ART also finishes as expected.

@astiob: I didn't notice this before, sorry, but shouldn't this have bumped LIBASS_VERSION in ass.h by 1 to make it possible to #if guard usage of the feature enum? (late, but we can still bump it now I guess)

Indeed, thank you. Pushed.