SCC: caption management refactoring #395
Conversation
valnoel
force-pushed
the
scc/buffer_management_refactoring
branch
from
82a097c
to
9cc73bf
Compare
| # Captions being displayed | ||
| self.active_caption: Optional[SccCaptionParagraph] = None | ||
| # Caption style (Pop-on, Roll-up, Paint-on) currently processed | ||
| self.current_style = SccCaptionStyle.Unknown |
There was a problem hiding this comment.
Shouldn't we have a "default" style?
There was a problem hiding this comment.
What would be this default style? It seems a bit tricky to choose...
There was a problem hiding this comment.
I skimmed the 608 spec and couldn't find an obvious answer.
I would choose paint-on for now.
| # Buffered caption being built | ||
| self.buffered_caption = None | ||
| # Captions being displayed | ||
| self.active_caption: Optional[SccCaptionParagraph] = None |
There was a problem hiding this comment.
Shouldn't we start with a non-empty active caption, esp. if the default style is paint-on?
There was a problem hiding this comment.
I preferred to consider that nothing is displayed at start. And as the active caption is the caption being displayed, I choosed not the initialize it at start. That's why the "begin timecode" is set into the new_active_caption method.
Otherwise, we should initialize the active caption without "begin timecode", but it would mean that an active caption is ready but not currently displayed, which doesn't make sense for me.
What do you think?
There was a problem hiding this comment.
Shouldn't the begin time code be set when the first printable character is displayed, instead of when the caption is instantiated?
There was a problem hiding this comment.
The idea is to make the start of processing the same as the middle, i.e. there is always a front buffer and a back buffer active.
Maybe this is too much of a change for now. Let me know.
There was a problem hiding this comment.
Shouldn't the begin time code be set when the first printable character is displayed, instead of when the caption is instantiated?
This is done for Paint-On only. The begin time must be set on EOC for Pop-On, and on CR for Roll-Up.
The idea is to make the start of processing the same as the middle, i.e. there is always a front buffer and a back buffer active.
I understand the idea, but I think we cannot start writing into buffers without a minimum of context (a style at least), otherwise we're moving away from the spirit of the 608 specification...
But if it's a prerequisite, the change shouldn't be too big...
There was a problem hiding this comment.
608/708 are in fact streaming format (SCC added the timestamps) and a device could join the stream at any time and thus possibly in the middle of a caption.
What is the current behavior if we start parsing in the middle of a caption? A possible behavior is to do nothing until a style is specified -- is that what you have in mind? Maybe we should add this to the unit tests?
There was a problem hiding this comment.
@palemieux I've changed the reader behavior so that it skips captions until a style is specified in 49d14db
Let me know what you think!
|
@atsampson @rktcc Your feedback would be welcome on this PR. |
|
@atsampson @rktcc Did you have the opportunity to review? The plan is to merge this PR sometime next week. |
valnoel
force-pushed
the
scc/buffer_management_refactoring
branch
from
e749eb9
to
9931fc3
Compare
valnoel
force-pushed
the
scc/buffer_management_refactoring
branch
from
ec30c3a
to
e236a54
Compare
Skip until a caption style is specified
Paragraphs have PopOn style and empty text line by default
valnoel
force-pushed
the
scc/buffer_management_refactoring
branch
from
e236a54
to
6f98d79
Compare
SCC reader refactoring proposal, addressing #383 & #385 issues