Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STL reader: option to set a generic, basic style #416

Closed
wants to merge 2 commits into from

Conversation

ngaullier
Copy link
Contributor

The EBU mapping is not always welcome. For simple distribution to end users, a more generic/simplified format appears to be sometimes a better choice.
The first patch disables the "ebu style".
The second disables the vertical positioning in favor of a fixed bottom position, with an adjustable margin.

README.md Outdated

Overrides line positions, force to bottom with defined safe margin in percents: 0 means sticked to bottom of display

Default: disabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does disabled mean since force_bottom_set_margin is a float?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default be None?

README.md Outdated
@@ -171,6 +181,14 @@ Specifies a maximum number of rows for open subtitles, either the MNR field of t

Default: `23`

#### force_bottom_set_margin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### force_bottom_set_margin
#### force_bottom_align_with_margin

@ngaullier
Copy link
Contributor Author

Thank you for the review, I am not good at namings and close to a newbee in python.
(Note: in the code, I have allowed int besides float because it makes sense from a user perspective, but I don't think this has to be mentioned in the doc).

@palemieux
Copy link
Contributor

palemieux commented Nov 21, 2023

@ngaullier I was looking at the results of disable_ebu_style:

"disable_ebu_style": true:
Screenshot 2023-11-21 091658

"disable_ebu_style": false:
ebu-styles

It is not clear to me how removing line padding and relaxing font size and line height makes it better for end-users.

Is this to address player incompatibility? If so, is it an incompatibility at the XML syntax level or with TTML features?

@ngaullier
Copy link
Contributor Author

It is a mix of things.
AFAIK, the "double height" feature for example, is not really used nowadays - or not for good reasons. All teletext based subtitles are meant for "double-height", at least for the french market I know well. I think it is really a full legacy stl feature which may be used in some niche scenario I am not aware of. When double height control is missing, it is either an "error" or a convention while the code is inserted later in the workflow if required for legacy interop. In a general manner, I don't like the idea of the double height control (and its related issues) be forwarded downstream to the TTML. If necessary, I think the fontsize should be fixed/updated directly in the TTML writer.
I have also in mind netflix "specs" as they are often read with attention even outside of the netflix "system"; they require fontsize 100% but I think it is default, so anyway my intend is to not go beyond "100% or default".
https://partnerhelp.netflixstudios.com/hc/en-us/articles/360000456768-What-does-a-properly-formatted-TTML-file-look-like-
As a broadcaster, I like the idea of having a simple single TTML file I can test/check operability once, and then I know that all other following subtitles won't be different. A single teletext block with unexpected formating can ruin a whole delivery, whereas user expectations are usually pretty low: consistent text formating, bottom centered, with just colors and at least horiz alignement for SDH. I would like all my TTML to look the same, whatever the STL input formating and whatever STL or vtt is used as input. And, for example, you can have a workflow with stl->vtt->ttml vs. another with direct stl->ttml and the two looks different, which is possibly unexpected by some users.
EBU did a great job with the TTML mapping, and it is helpful in contribution/B2B scenario where TTML exactly replaces STL, but I don't think that it should be used for delivery to end users.

@palemieux
Copy link
Contributor

Thanks for the details. It does feel like multiple separate issues:

  1. for double height, maybe we should have a force_double_height in case it is missing by convention rather than intentionally -- this is an STL-specific issue

  2. overriding font size, line height and paragraph positioning (a simple single TTML file) is arguably applicable to any input, not necessarily STL

For (2), the plan has been to allow the user to specify filters to be applied between the reading and the writing steps. This would conceivably allow the creation of a, say, Netflix filter that would remove features prohibited by Netflix, regardless of the input.

Makes sense?

The mechanics for implementing such filters mostly exists and just needs to be exposed at the CLI level.

@ngaullier
Copy link
Contributor Author

I like the idea but technically speaking, it seems like an enormous work (out of my own limited python skills).
I have already thought of using the existing merge_regions filter to see how I could use the logic internally (without exposing to cli) to force a single output region, but I did not find a way to do it.
The hardest part I identified is that VTT/SRT writers actually use filtering (at least because not all ttml features can be mapped to it), whereas the intermediate format, the model, is quite a generic map of the ttml syntax, and thus the ttml writer use another logic.
VTT writer:
def from_model(doc: model.ContentDocument ... :
isds = ISD.generate_isd_sequence(doc, _isd_progress)
TTML writer:
def from_model( model_doc: model.ContentDocument ... :
imsc_elements.TTElement.from_model(model_doc, ...

So I don't catch how I could make the TTML writer use ISDs or any other way to make filters available to ttml output ?

@palemieux
Copy link
Contributor

I see two different but related types of filters:

  • filters acting on the ISDs created by some of the writers -- in addition to any filters applied by these writers
  • filters acting on the document before it is provided to the writers

I think I have a draft branch somewhere implementing the framework.

Would you be available to test the framework and potentially help create practical filters?

@ngaullier
Copy link
Contributor Author

Yes, I will try. I would think I won't be helpful for the creation of the framework itself, but testing and creating filters should not be a big issue for me, I guess.
I think we share quite the same perception. To precise a bit, I would say ISDs filters would remain unchanged, maybe sticked to an internal thing, they have proven to be useful to handle "supported features" and make the body code more simple because of removing testing conditions.
The new filters would be diverse, including transforming filters to force position or size, or restricting the charset : it would be nice if they could process strings, I would say. Maybe even prepare the ground for a qc filter : typically, "warn me if a subtitle has more than 2 lines or more than 52 chars".

@palemieux
Copy link
Contributor

I will take a stab at the framework soon

@ngaullier
Copy link
Contributor Author

I fear it may take a long time to complete the whole thing. Do you think it could be possible to have a first step, which would implement the definitive cli of course but with an intermediate internal code ? I mean "disable_ebu_style" would be renamed sth like "skip_formating" and both options with "force_bottom_align_with_margin" would fill in a new "filter" overall options block; they would be mapped as STL parser options at a first step, but ready to be moved to a new schema later on when available ?
This would have the benefit to have real use cases to start with, with regression tests already existing, so smoother transition.
But maybe you will feel it an unacceptable temporary hack ?

@palemieux
Copy link
Contributor

Sorry for the radio silence. I have spent quality time with the document filter framework and have concluded (I think) that it is not possible to generally split the problem of filtering a document into multiple independent filters. For example,

  • changing the text color does not make sense without also changing the background color of all parent elements
  • increasins the font size is not wise without also cleaning-up regions to make sure they are large enough
  • repositioning regions is not possible without first merging regions
  • etc.

A preferable approach would be to define filters that target a specific profile/delivery spec. The implementation of each of these filters could reuse building blocks, e.g. merge regions.

Does this make sense?

If so, can you describe the kind of document that you would like to end up with, i.e. the delivery spec that you are targeting? For example:

  • are multiple regions allowed?
  • is any text style/decorations allowed?
  • is background color allowed/specified?
  • etc.

Based on that I can take a first stab that you would be able to modify.

@ngaullier
Copy link
Contributor Author

Thank you for the feedback. I think most users are focused on delivery spec rather than custom processing rules, so this would be fine.
My own use case is very limited: input is a closed caption STL (no style/decoration, only color) with vertical line positioning ignored, and output is vtt or ttml with same rendering, so:

  • single region (bottom, bottom aligned) [ but netflix seems to support top and center, so three regions ]
  • no bold, italic or so, only text color (for SDH) in the inputs [ but using open caption STL with styling is pretty commonplace for forced subs, so should pass-through ]
  • the text can be left/right/center aligned for SDH
  • background is simple black ; nevertheless, there could be an issue with opacity(interop; I had some very bad experience with DVBsub so always suspicious since even with very different devices here), so it may require customization
  • font size = default (not sure it is always used...)

@palemieux
Copy link
Contributor

palemieux commented Dec 17, 2023

@ngaullier I have take a stab at [introducing document filters](filters: #423) and would appreciate your feedback. The initial filter is the "lcd" filter, which merges regions and removes all styles but text color.

tt convert -i <.scc file> -o <.ttml file> --itype SCC --otype TTML --filter lcd --config '{"general": {"progress_bar":false, "log_level":"WARN"}, "lcd": {"safe_area": "10"}}'

The PR needs to be cleaned-up a little bit, but the core functionality is there.

If ok, we should close this PR and move the discussion to that other one.

@ngaullier
Copy link
Contributor Author

It seems great, I will get more time to look at this tomorrow. Just a first quick feedback: the naming rules between isd- and doc- filters may need to be split, and the implementation of the supported style props filters could be unified. I tried a draft branch to play with it and see what it would imply, you can have a quick look at https://github.com/ngaullier/ttconv/tree/refactor_supportedstylefilters
Unifying makes the warning "supported_style_properties.py has less than 80% coverage" disappear without further work, which is nice.

@palemieux
Copy link
Contributor

Just a first quick feedback: the naming rules between isd- and doc- filters may need to be split, and the implementation of the supported style props filters could be unified.

Yes, that is the plan. I can taken care of this if you can check if the document filter matches your expectations.

@ngaullier
Copy link
Contributor Author

It is really super great.
"Major" thing is the command line syntax:
--filter xx1 xx2 xx3
If you don't mind changing the nargs=* to something like:
argument("--filter", help="Document filter", required=False, action="append"),
That would allow a more usual syntax:
--filter xx1 --filter xx2
Another option: something like --filter "xx1,xx2" would be similar to a standard ffmpeg lavfi.


The "lcd" filter is just great itself. (BTW, I think its name is kind of mysterious for a user, so maybe the acryonym should be placed somewhere in the readme).
I think it should support the StyleProperties.TextAlign in the supported_styles, at least by default (and that would also fully cover my use case with no other addition).
What would be even greater in the future would be to customize the supported_styles via config.
I checked that I can very very easily chain the lcd filter with a second custom document filter, for example to set a fixed font size or anything. This is great.
(I understood you are currently cleaning everything, so you certainly already get the colors/config/defaults issue, and noticed the process() just returns void/no ContentDocument)

Thank you very much. Your tool is already great and this feature will make it even greater.

@palemieux
Copy link
Contributor

That would allow a more usual syntax:

Doh! That was my intent :)

I think it should support the StyleProperties.TextAlign in the supported_styles, at least by default (and that would also fully cover my use case with no other addition).

+1

What would be even greater in the future would be to customize the supported_styles via config.

"Later"

@palemieux
Copy link
Contributor

@ngaullier I would appreciate your review on #423 . I have bumped the version to 1.1.0 since we are breaking internal contracts.

@ngaullier
Copy link
Contributor Author

ngaullier commented Dec 26, 2023

In lcd, bg_color works but not color, I think that direct "set_style" should be replaced by an "_apply_color" recursion like bg_color.
From a user point of view, the usage of bg_color is not plain easy. Two possibilities:

  • either document explicitly that using #00000000 will "clear" the bg_color in case one want to get totally rid of it
  • or make the default be None, likewise color

I think the latter more straightforward as it does not require additional documentation.
A README entry is missing for the latest option "preserve_text_align".
(BTW, since the removal of some unused "getLogger" in filters, the whole "import" can now be removed too. And lcd/process returns no value anymore)
I forget to say previously, but the kind of "plugin-mechanism" for the doc filters is really "nice to have". It allows easy hotfixes in a production environment for example, or ability to implement a turnaround while waiting for one's PR.
Thanks for your work.

@palemieux
Copy link
Contributor

In lcd, bg_color works but not color,

Do you mean that setting color in the config does not override text color?

From a user point of view, the usage of bg_color is not plain easy. Two possibilities:
or make the default be None, likewise color

By None, you mean transparent? What do you think should be the default, black or transparent background?

@ngaullier
Copy link
Contributor Author

ngaullier commented Dec 28, 2023

Yes, setting color does not override text color.
Take a simple cmdline: convert -i xx.stl -o xx.ttml --filter lcd --config '{"lcd": {"bg_color":"red", "color":"blue"}}'
=> xx.ttml will have tts:backgroundColor (at paragraph level) but no text color (incoming STL text color at span level is removed but no new overriden style is shown).
It can be fixed by using the same logic for color as the one of bg_color (I tried ngaullier@c3e8761), but I don't know if this sounds "right" to you.

Sorry, my words were misleading.
Ideally, for fg/bg colors, I think the "wider" possibilities the best, so be able to either pass-through, or force a specific code. To do so easily, the default should be to pass-through, ie. python None (or the user option would have to be a little more sophisticated to handle a 'passthrough' code which would be mapped to None). You already implemented it for text color and I think it should be the same for background color.
And I think the "transparent" TTML color code should be highlighted in the documentation for the background color; maybe just simply put in the existing example command line. And this example could also use the syntax #112233FF for the text color to make the user fully understand what is behind the "" parameter, despite this is all standard.

Last thing:
--config '{"lcd": {"bg_color":"red"}}'
--config '{"lcd": {"color":"blue"}}'
--config '{"lcd": {}}'
will raise an exception (TypeError in parse_color): there must be a trick in the config class, I don't know where.
But setting both like
--config '{"lcd": {"bg_color":"transparent", "color":"blue"}}'
is okay

@palemieux
Copy link
Contributor

Yes, setting color does not override text color.

convert -i src/test/resources/ttml/imsc-tests/imsc1/ttml/color/Color001.ttml -o build/color.ttml --filter lcd --config '{"lcd": {"bg_color":"red", "color":"blue"}}' seems to work: it sets tts:color on body from which all descendents inherit.

Do you have a specific example?

You already implemented it for text color and I think it should be the same for background color.

Ok.

nd I think the "transparent" TTML color code should be highlighted in the documentation for the background color;

Ok.

will raise an exception (TypeError in parse_color): there must be a trick in the config class, I don't know where.

Ok.

@ngaullier
Copy link
Contributor Author

Maybe it is specific to ebu stl input, I have not tested broadly at all. You can try this file for example, but there should not be anything exotic with this file, except it is ebu stl.
short888.zip

@palemieux
Copy link
Contributor

This is what I get:

image

@ngaullier
Copy link
Contributor Author

Indeed! Seems I have to buy some new eyes and I did not read your answer carefully enough: the body, yes! Sorry about that. It is ok.

@palemieux
Copy link
Contributor

palemieux commented Dec 28, 2023

@ngaullier I think I have addressed your feedback (many thanks) with the latest commit. It would be good if you could add your review to #423

@palemieux
Copy link
Contributor

Replaced by #423

@palemieux palemieux closed this Dec 28, 2023
@ngaullier ngaullier deleted the stl_generic branch March 19, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants