Improved EBU-TT-D encoding, including better tests and denester #523
Conversation
Python3 migration staging branch
Closes #490. Calling fixtures directly is deprecated, this solution described at https://docs.pytest.org/en/latest/deprecations.html#calling-fixtures-directly seems to work, creating a named fixture rather than defining the "then" step as a fixture directly.
This was added accidentally in error, and has no meaningful effect except to cause some downstream problems. No tests seem to be missing that would use this functionality, and all tests pass without it, so we think it is unnecessary.
| @@ -192,6 +192,20 @@ def __init__(self, config, local_config): | |||
| self._create_input(config) | |||
| self._create_output(config) | |||
|
|
|||
| class Denester(ConsumerMixin, ProducerMixin, NodeBase): | |||
There was a problem hiding this comment.
The class has to be referenced further down in nodes_by_type, in order to be used e.g. in a config file.
There was a problem hiding this comment.
Agreed, I think we have some updates to make to this pull request based on further work, and this is one of them.
| ) | ||
|
|
||
| def __init__(self, config, local_config): | ||
| super(RetimingDelay, self).__init__(config, local_config) |
There was a problem hiding this comment.
This should be Denester instead of RetimingDelay ;-)
There was a problem hiding this comment.
Yikes! Good spot, thanks for that. Weird python, I wonder what it actually does when we write it like this?!
There was a problem hiding this comment.
Python then aborts with a type error:
TypeError: super(type, obj): obj must be an instance or subtype of type
There was a problem hiding this comment.
Oh I see, fair enough. Weird that we never hit that bug.
| unnested_divs = DenesterNode.combine_divs(unnested_divs) | ||
| unnested_divs = DenesterNode.check_p_regions(unnested_divs) | ||
| document.binding.body.div = unnested_divs | ||
| return document |
There was a problem hiding this comment.
Testing the Denester, no output documents are generated at all here. As far as I can see, returning the document is not enough but instead self.producer_carriage.emit_data has to be called to forward the processed document.
There was a problem hiding this comment.
Agreed, I see that I have some changes made on the BBC fork that include a fix for that, so I will push an update to this pull request.
| @@ -125,6 +125,9 @@ Node type dependent options for [nodeN] : :: | |||
|
|
|||
| type="distributor" : No options | |||
|
|
|||
| type="denester" | |||
| └─sequence_identifier : sequence identifier, default "Denester1" | |||
There was a problem hiding this comment.
This option is not (yet) supported by the Denester.
There was a problem hiding this comment.
Again, to be fixed by pushing more commits here - we have this fixed in the BBC fork. Apologies!
There was a problem hiding this comment.
OK, no problem! So I will put my review on hold, until I get news from you regarding this MR.
Add detailed conversion tests to improve the EBU-TT Live to EBU-TT-D conversion.
This also adds a Denester node that flattens nested div and span elements, which meets a requirement of EBU-TT-D.
This work represents contributions from a number of BBC engineers - thank you!