* Impose positive length constraints on data model to match IMSC#486
* Impose positive length constraints on data model to match IMSC#486
Conversation
|
@eko I would appreciate your quick review of this PR. The algorithm for converting WebVTT cue positioning to IMSC region positioning has been modified, for the better I hope. |
eko
left a comment
eko
left a comment
There was a problem hiding this comment.
Hi @palemieux,
This VTT reader rewrite looks like a significant improvement, the ipd/bpd decomposition is much cleaner, and the positioning logic seems more correct.
I've converted a few subtitle files I had on hand, and the results look correct :-)
The style_properties.py changes are in the same direction we were heading (enforcing positive lengths at the model level).
I've left a few comments, I'm not sure about all of them, but I hope they can be helpful for your review.
src/main/python/ttconv/vtt/reader.py
Outdated
| value = cue_settings.get("align") | ||
| if value in ("left", "right", "start", "center", "end"): | ||
| cue_text_align = value | ||
| else: |
There was a problem hiding this comment.
What about moving to elif value is not None: here?
Because when align is not in the cue settings, value is None, which falls into else and logs "Bad align setting value: center". The old code had elif value is not None: to guard against this.
| if not isinstance(self.units, LengthType.Units): | ||
| raise ValueError("Invalid units") | ||
|
|
||
| def is_positive(self): |
There was a problem hiding this comment.
💡 is_positive() returns self.value >= 0, which is mathematically "non-negative", not "positive". is_non_negative() would be clearer.
There was a problem hiding this comment.
In my mind, value > 0 is strictly positive, so that value >= 0 is positive :)
... but non-negative is completely unambiguous.
src/main/python/ttconv/vtt/reader.py
Outdated
| # position value | ||
| cue_position = parse_vtt_pct(value[0]) | ||
| if cue_position is None: | ||
| LOGGER.warning("Bad position alignment setting value: %s", cue_position_align) |
There was a problem hiding this comment.
cue_position_align looks to be None at this point, I think you wanted to use cue_position here?
| def validate(value: Numeric): | ||
| return isinstance(value, numbers.Number) | ||
|
|
||
| return isinstance(value, numbers.Number) and value > 0.0 |
There was a problem hiding this comment.
💡 I am not sure about this one but TTML spec says that luminanceGain is a non-negative number and here it's strictly positive, maybe this value should be >= 0.0?
https://www.w3.org/TR/2018/REC-ttml2-20181108/#style-attribute-luminanceGain
There was a problem hiding this comment.
tts:luminanceGain == 0 is absurd, but, as you say, syntactically permitted in TTML 2!
|
@eko Thanks so much for the review. I plan to release a beta version shortly. |
2 participants
Closes #483
Closes #487