-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add media.ccc specific parsing of recordings #1250
base: dev
Are you sure you want to change the base?
Conversation
Instead of pushing the `media.ccc` recordings straight into the StreamExtractor straightjacket (which does not really provide the correct API for handling the semantics of the media.ccc data), we parse into an intermediate structure first that contains all relevant information about the different streams. This is required for correct handling of the different languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review, I think this PR is well organized. Thanks!
Btw, please do not forget to fill in or dismiss the checkboxes in the PR description.
} | ||
|
||
return videoStreams; | ||
} | ||
|
||
public List<MediaCCCRecording> getRecordings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review. You should probably cache the value returned by getRecordings()
to avoid recalculating it in getVideoStreams()
, getAudioStreams()
and getVideoOnlyStreams()
.flatMap(r -> | ||
r instanceof MediaCCCRecording.Video | ||
? Stream.of((MediaCCCRecording.Video) r) | ||
: Stream.empty() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also in other places)
.flatMap(r -> | |
r instanceof MediaCCCRecording.Video | |
? Stream.of((MediaCCCRecording.Video) r) | |
: Stream.empty() | |
) | |
.filter(MediaCCCRecording.Video::isInstance) | |
.map(MediaCCCRecording.Video::cast) |
MAIN, | ||
/** A side-recording of a talk/event that has the slides full-screen. | ||
* Usually if there is a slide-recording there is a MAIN recording as well */ | ||
SLIDES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should add something like this but for videos, to distinguish between the main and secondary types
Instead of pushing the
media.ccc
recordings straight into the StreamExtractor straightjacket (which does not really provide the correct API for handling the semantics of the media.ccc data), we parse into an intermediate structure first that contains all relevant information about the different streams.This is required for correct handling of the different languages.
It should not change the API, since its specfic to the media.ccc Extractor. I just type-cast in frankenplayer at the moment.
I skipped adding getters/setters in the MediaCCCRecording structs, once this repository is migrated to Kotlin (?), it would just be a data class.