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

Missing test for #region-timing. #203

Closed
cconcolato opened this issue May 2, 2019 · 11 comments
Closed

Missing test for #region-timing. #203

cconcolato opened this issue May 2, 2019 · 11 comments
Labels
no change Closing without change (see comments).
Milestone

Comments

@cconcolato
Copy link

I tried to search for tests exercising #region-timing in:

@css-meeting-bot
Copy link
Member

The Timed Text Working Group just discussed Missing test for #region-timing #1073, and agreed to the following:

  • SUMMARY: @cconcolato to create a pull request on imsc-tests adding a region timing test, @palemieux to run the test through imsc.js
The full IRC log of that discussion <nigel> Topic: Missing test for #region-timing #1073
<nigel> github: https://github.com/w3c/ttml2/issues/1073
<nigel> i/Topic/scribe: nigel
<nigel> Nigel: region timing is pretty complex, and there are no tests, right?
<nigel> Cyril: I checked and couldn't find them, and neither could Pierre
<nigel> Pierre: Nope, I couldn't
<nigel> Nigel: Given the complexity of this I think it's something we should work on, or even deprecate the feature.
<nigel> Cyril: It's used for IMSC Image Profile?
<nigel> Pierre: That's the only place I've ever seen it used.
<nigel> Cyril: How is it used?
<nigel> Pierre: A div is selected in a region and the div has the smpte:backgroundImage, and there's a 1:1 mapping
<nigel> .. between each div and each region.
<nigel> Cyril: Why is the timing on the region and not on the div?
<nigel> Pierre: I literally don't know. I remember seeing it once done that way.
<nigel> Cyril: It's not a pattern?
<nigel> Pierre: I don't think so. [checks out the Fox content]
<nigel> .. In their example the timing is on div.
<nigel> .. I remember seeing this used once but I don't think it's a pattern.
<nigel> Cyril: That one has div timing
<nigel> Pierre: Right
<nigel> Nigel: There are subtleties here:
<nigel> .. For example, the ISD synthesis rehomes body to a region parent, but the body timings are not (I think)
<nigel> .. relative to the region's timings. Is that right?
<nigel> .. Therefore there's a potential intersection between the active intervals of a region and content selected into it.
<nigel> .. So what happens to content selected into a region outside its active interval?
<nigel> .. Does it get temporally clipped? Or does the region's active interval get extended?
<nigel> .. Pierre, you must have implemented this, but I can easily imagine other implementers might get it wrong one way or another.
<nigel> Pierre: [looks at the imsc.js implementation]
<nigel> .. It's pretty clear that my conclusion at the time was that regardless of how timing was set on a region, if it was not
<nigel> .. active then nothing else can be active at that time.
<nigel> Nigel: So the region timing temporally clips its content?
<nigel> Pierre: Yes. I think there was a thread on this, I'd have to go back.
<nigel> .. This makes sense in the context of showBackground="whenActive". That's one way to have a region where
<nigel> .. the black background is only shown when the region is active, and making that explicit.
<nigel> Nigel: That does make some sense, yes.
<nigel> Pierre: One way to set when a region is active explicitly, especially in the US caption style, is to have a begin and end
<nigel> .. on a region.
<nigel> Nigel: You'd think that, however the normative text on showBackground refers to content being selected into it.
<nigel> .. So arguably even if a region is active, if no content is flowed into it then showBackground doesn't happen.
<nigel> Pierre: The way imsc.js does it is it resolves timing completely separately for regions and body. It applies the timing
<nigel> .. resolution algorithm separately to region and to body. So it's the intersection model.
<nigel> Nigel: That's easily testable, right?
<nigel> Pierre: Right. I was stunned not to find a test in the imsc tests, because I thought there was one, but maybe no test
<nigel> .. ever made it to the test suite. I searched and couldn't find any region with a begin on it.
<nigel> .. I really like the idea of adding tests.
<nigel> Cyril: The related question is what to do on IMSC. I opened an issue to deprecate this feature in IMSC, w3c/imsc#473.
<nigel> Pierre: In the grand scheme of things it's not a big deal if the intersection model is used, it's pretty easy to explain
<nigel> .. and straightforward. And it's implemented in imsc.js.
<nigel> .. One thing to think about is there are tons of other things that nobody should use, like nested timing.
<nigel> .. Maybe one thing to consider is to at some point in the IMSC lifecycle do a pass on everything that we think people should not do
<nigel> .. because there's no use for it and deprecate all of them. It would be weird to deprecate just that one.
<nigel> Cyril: If you find other features to deprecate then fine, but we should give a signal and not let it linger.
<nigel> Pierre: Why does it shock you, aside from the lack of tests?
<nigel> Cyril: We don't implement it and don't see the value.
<nigel> Pierre: That's true of other things, right, like seq timeContainer, nested region reference (e.g. nested divs with different region references)?
<nigel> .. I've not found any reason why anyone would do the latter, which has serious consequences.
<nigel> .. I have my own personal list!
<nigel> Cyril: We should go through that list. If people don't implement it, then let's be clear about it.
<nigel> Pierre: My suggestion is if we want to start deprecating things that are not broken but we think are not useful and
<nigel> .. potentially confusing we should look at that. It's late in this cycle.
<nigel> Cyril: If it has never been tested you can't say it's not broken, right?
<nigel> Pierre: I don't think that argument really works. We can agree it's not great to use.
<nigel> Cyril: If you cannot rely on it because implementations don't agree, then either it's broken or deprecate it.
<nigel> Pierre: TTPE and imsc.js might actually work but that's not the right threshold.
<nigel> .. Just because they implement it doesn't mean it shouldn't be deprecated.
<nigel> Cyril: No, but if they don't agree then we should consider deprecating it.
<nigel> Pierre: My point is there are a lot of features that go into whether or not to deprecate something.
<nigel> Cyril: Exactly.
<nigel> Nigel: The first stage is to create a test to see if the implementations agree or not.
<nigel> Cyril: Yes
<nigel> Pierre: I think that's a great idea.
<nigel> .. I'm supportive of generating a list of features to deprecate from IMSC but we might be late in this process to do that.
<nigel> Nigel: At least surfacing that list would be helpful even if we don't implement the deprecations until later.
<nigel> Pierre: Totally agree.
<nigel> Nigel: Does anyone want to create the tests?
<nigel> Pierre: We can split the burden. Cyril, if you want to create the tests then I can add them to imsc-tests.
<nigel> .. I recommend you create a pull request against imsc-tests and then I'll test them with imsc.js.
<nigel> Cyril: Hmm, creating the tests when I don't support the feature!
<nigel> Pierre: The primary ingredient for deprecation is that nobody wants it.
<nigel> .. I'm not going to come back and say because you created the test you want the feature!
<nigel> Cyril: Yeah, I'll create the test.
<nigel> Pierre: By the way you might prove it's broken, in which case we can deprecate it urgently.
<nigel> SUMMARY: @cconcolato to create a pull request on imsc-tests adding a region timing test, @palemieux to run the test through imsc.js

@skynavga
Copy link
Contributor

skynavga commented May 9, 2019

Note step (2) of construct intermediate document.

@nigelmegitt
Copy link
Contributor

@skynavga thanks, my reading of that is that the temporal activation interval of a region acts as a temporal clipping window for all of the content that would otherwise be flowed into that region, so the discussion earlier shows that the implementation intent matches the specification, and we should expect tests to pass, when they have been written.

@skynavga
Copy link
Contributor

skynavga commented May 9, 2019

@cconcolato agreed

@skynavga
Copy link
Contributor

skynavga commented May 23, 2019

[deleted - see comment history]

1 similar comment
@skynavga
Copy link
Contributor

skynavga commented May 25, 2019

[deleted - see comment history]

@skynavga skynavga transferred this issue from w3c/ttml2 May 30, 2019
@skynavga
Copy link
Contributor

I would like to point out that the current set of TTML2 tests consist of only those tests required to satisfy CR exit criteria, and, as such, are exclusively designed to test new or changed functionality. Therefore, given that region timing was defined in TTML1, I question whether or not we should consider creating new ttml2 tests to serve this purpose. More likely, we should replace the ttml1-tests repository with a new set of tests that do a more thorough job of testing features defined in TTML1.

@palemieux
Copy link

FYI. A test was added to the IMSC test suite: w3c/imsc-tests#86

@css-meeting-bot
Copy link
Member

The Timed Text Working Group just discussed Missing test for #region-timing. ttml2-tests#203, and agreed to the following:

  • SUMMARY: @nigelmegitt to raise an issue on ttml1-tests to add the region timing tests segregated away from CR exit criteria tests
The full IRC log of that discussion <nigel> Topic: Missing test for #region-timing. ttml2-tests#203
<nigel> github: https://github.com//issues/203
<nigel> Nigel: We don't have Cyril here, maybe it wasn't a good candidate for the agenda, let's see if we can do anything...
<nigel> Nigel: I see that there are IMSC tests for this TTML1 functionality; can we simply copy those tests into the ttml1-tests
<nigel> .. repo from the imsc-tests repo?
<nigel> Glenn: The TTML1 tests are in need of some work. I consider them legacy at this point.
<nigel> .. They need more work and I'm not sure how to go about doing that?
<nigel> .. If you start copying in some tests it will create a ripple in what's there right now. I urge some caution to consider the ramifications
<nigel> .. and the bigger picture for TTML1 testing. Keep in mind that the tests we have in our test suite are not for interop testing
<nigel> .. but for spec completion testing to get out of CR. I'm not sure if those new tests should go into TTML1 or what category
<nigel> .. they are in. Are they testing existing TTML1 functionality for which there was no test in the first place?
<nigel> Nigel: I'd say they're testing functionality in TTML1 that didn't have tests before.
<nigel> .. Bear in mind that we rely on TTML1 tests for TTML2 also, for features defined in TTML1.
<nigel> .. Also if this could be even a small improvement I think we should do it even if it doesn't address the whole set of problems with the TTML1 tests repo.
<nigel> .. I'll take an action to raise an issue on ttml1-tests where we can consider including these tests.
<nigel> Glenn: Just to let you know I might feel obligated to object on the basis that it extends the set of tests for existing
<nigel> .. features beyond what the CR tested for. I wouldn't mind including it if we could segregate additions in some way.
<nigel> .. I haven't got a suggestion.
<nigel> Nigel: I'd be happy to segregate it in some way.
<nigel> Glenn: That would make me a lot more comfortable.
<nigel> .. I don't know the state of documentation on the TTML1 test suite but we should document inclusions like this in a
<nigel> .. sensible way that point out that this is not intended to revisit the tests for exiting CR.
<nigel> Nigel: I understand the concern, I think we'll be okay.
<nigel> SUMMARY: @nigelmegitt to raise an issue on ttml1-tests to add the region timing tests segregated away from CR exit criteria tests

@nigelmegitt
Copy link
Contributor

Further to the above, I looked at the ttml1-tests repo and observed that it doesn't contain any tests. This means it is straightforward to develop it as a new non-CR-exit-criteria test suite for general use while leaving the CR exit criteria test suite in its current location. I raised w3c/ttml1-tests#1 to propose this approach, including adding a statement in the README to point out that it isn't the CR exit criteria test suite.

@skynavga
Copy link
Contributor

skynavga commented Sep 1, 2019

Closing with further action in this repository since a test has already been added in w3c/ttml1-tests#6.

@skynavga skynavga closed this as completed Sep 1, 2019
@skynavga skynavga added pr merged no change Closing without change (see comments). and removed agenda On table for meeting agenda. pr merged labels Sep 1, 2019
@skynavga skynavga added this to the 2ED milestone Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no change Closing without change (see comments).
Projects
None yet
Development

No branches or pull requests

5 participants