search

opencast / annotation-tool

A video annotation service that is suitable for research, teaching and learning
Educational Community License v2.0
40 stars 19 forks source link

fix loading when 'video' key is missing #629

Closed jduehring closed 1 month ago

jduehring commented 2 months ago

After updating to OC 16.4 we had the issue that the videos did not load in the OAT. @luniki found out that it is related to HLS tracks missing the "video" key. The changes in this PR fix this by only using tracks that have the correct keys.

We are however unsure as to why this was no issue with older versions of the OAT.

JulianKniephoff commented 2 months ago

We are however unsure as to why this was no issue with older versions of the OAT.

This is probably because accessing/passing along these properties is new in the next branch: https://github.com/opencast/annotation-tool/commit/8397285fc1a68e04900c4e1dc795596be3c22d4a

I don't know why it was added in the first place, maybe @ComeIn-NRW and/or @github-canni can shed some light? The commit message unfortunately doesn't say.

JulianKniephoff commented 1 month ago

I took the liberty of actually removing the fields since I could find no good reason for us to pass them around in MediaElement.js. 🤷‍♀️ They probably came in as part of some unfinished business during the "Münster-merge."

cf. bf39a876494ce21a9b6db7094e420bc199d72607

oellers commented 1 month ago

I can shed some light on this. The commit comes originally from an internal branch that made the annotation tool xAPI-compatible. This also required frame rate and resolution (as we built upon the xapi cop video profile). At some point this was functional as we tested the xAPI statements. We also tested an older version of opencast against the OAT "next", so there might have been a change in opencast itself?

If there's interest in xAPI for the annotation tool, we may work on an implementation for the next version of the tool at some point in the future. Removing the fields won't break anything at this point, but if xAPI is implemented in the future, they will probably be added back.

JulianKniephoff commented 3 weeks ago

Interesting, thanks for the info! :D