Video MP4 output #646
Video MP4 output #646
Conversation
WojciechBarczynski
changed the base branch from
master
to
@WojciechBarczynski/mp4
There was a problem hiding this comment.
I'll refactor types once I know what is needed for AAC decoding.
There was a problem hiding this comment.
This'll be modified in next PRs
|
I'll address review suggestions in the next PRs |
| std::thread::Builder::new() | ||
| .name(format!("mp4 writer thread for output {}", output_id)) | ||
| .spawn(move || { |
There was a problem hiding this comment.
TODO: Add here log span
There was a problem hiding this comment.
Usually when creating a feature branch that will later merged I would create a branch without @username prefix. I would expect branches like this to be open for cooperation.
I know that I also merge stuff like this without waiting for review, but I'm trying to do that only if:
- I can't expect review withtin 1-2 days
- I have already multiple PRs stacked one on the other
- I have a reasonable expectation that there wont be any changes as a result of the review and they will be cosmetic.
Merging stuff too early even to your own branch is causing that some of the changes that should be in this PR will be part of the AAC PR or totally separate one. It's not a problem for cosmetic changes, but if it is significant it defeats the purpose of spliting this feature into separate PRs
| @@ -34,6 +34,7 @@ pub use component::View; | |||
| pub use component::WebView; | |||
|
|
|||
| pub use register_input::Mp4; | |||
| pub use register_output::Mp4Output; | |||
There was a problem hiding this comment.
If this is a user-facing value, use Mp4 too. If it not user-facing, I would rename input mp4 for consistency
| @@ -93,7 +93,7 @@ fn run_encoder_thread( | |||
| let chunk = EncodedChunk { | |||
| data, | |||
| pts: batch.start_pts, | |||
| dts: None, | |||
| dts: Some(batch.start_pts), | |||
| @@ -160,32 +173,36 @@ impl OutputOptionsExt<RawDataReceiver> for RawDataOutputOptions { | |||
| impl Output { | |||
| pub fn frame_sender(&self) -> Option<&Sender<PipelineEvent<Frame>>> { | |||
| match &self { | |||
| Output::Rtp { encoder, .. } => encoder.frame_sender(), | |||
| Output::EncodedData { encoder } => encoder.frame_sender(), | |||
| Output::Rtp { encoder, .. } | |||
There was a problem hiding this comment.
personal opinion(here and below), but I think is less readable than the previous version and very annoying when you need to change anything
| EncoderOutputEvent::AudioEOS => playing_streams -= 1, | ||
| EncoderOutputEvent::VideoEOS => playing_streams -= 1, |
There was a problem hiding this comment.
Storing whether EOS was received via number seems wrong, if semantically this is bool it either was received or not. I would use Option<bool> per stream where None represents no stream, Some(true) received EOS, Some(false) not EOS yet
| EncoderOutputEvent::VideoEOS => playing_streams -= 1, | ||
| }; | ||
|
|
||
| if playing_streams == 0 { |
There was a problem hiding this comment.
With above suggestion this would be
if received_audio_eos.unwrap_or(true) && eceived_video_eos.unwrap_or(true) {
| .field("pts", &self.pts) | ||
| .field("dts", &self.dts) | ||
| .field("kind", &self.kind) | ||
| .finish() |
There was a problem hiding this comment.
I usually in overrides like this print len of the data that is not printed
|
|
||
| #[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct OutputVideoOptions { |
There was a problem hiding this comment.
You can't unify those values, after you add AAC here it will be incorrect for RTP. Start by duplicating and unifying when you know you can support unified interface everywhere.
There was a problem hiding this comment.
- After implementing AAC I want to add support from AAC audio output in both: RTP and MP4
- I purposefully left types refactoring for later. AAC encoder has a lot of options and I need to test which of them should be available.
- Since writing opus audio into MP4 via FFmpeg didn't work and I couldn't easily debug it, I want to implement AAC encoding first, then debug what's wrong / test if it works with AAC, and modify types later on (maybe writing opus into MP4 won't be available).
The video output works fine.
Opus audio fails to write file trailer and logs an
invalid extradata sizeerror.I'll implement an AAC encoder in another PR and test it after.
Merging:
I created the
@WojciechBarczynski/mp4branch to which I'll merge all MP4 output-related PRs. Then I'll merge it tomasterwithout squashing.PRs adding AAC encoder, MP4 outputs, etc. are large enough to be separate PRs/commits, but I don't want to merge to master before testing it out.