Add MJPEG support #88
Conversation
There was a problem hiding this comment.
Apologies for taking so long to get back to you on this. It looks really nice! I have some minor comments below.
It didn't fit in perfectly with the existing architecture (would maybe be nicer to add a new
ImageParametersstruct), but I think it's good enough?
How would ImageParameters differ from VideoParameters? We can do an API revision if necessary, but fwiw I don't see anything too wrong with using VideoParameters.
|
|
||
| use super::{VideoFrame, VideoParameters}; | ||
|
|
||
| const MAX_FRAME_LEN: usize = 2_000_000; |
There was a problem hiding this comment.
I'm guessing this is something you picked, rather than specified in the standard? Probably fine for now, but I wonder if we should have a mechanism to plumb things like this in.
There was a problem hiding this comment.
Yeah, I wanted some extra protection against corrupted bitstreams and just picked a large number.
src/codec/jpeg.rs
Outdated
| } | ||
|
|
||
| precision = 0; | ||
| } else if qtable.is_none() { |
There was a problem hiding this comment.
This seems redundant; isn't it covered by the check on line 448? Also, it seems like length/qtable/precision are only relevant in the frag_offset == 0 case; might be a bit more elegant to combine the two if blocks describing that (361 and 403) into one that produces a non-Option qtable before doing the lines 423-445 stuff.
There was a problem hiding this comment.
Good catch! Cleaned it up a bit
src/codec/jpeg.rs
Outdated
| if length > 0 { | ||
| qtable = Some(payload.clone()); | ||
| } else { | ||
| qtable = self.qtables[q as usize].clone(); |
There was a problem hiding this comment.
Should this check the q=255 special case? RFC 2435 section 3.1.8 says:
A Q value of 255 denotes that the quantization table mapping is dynamic and can change on every frame. Decoders MUST NOT depend on any previous version of the tables, and need to reload these tables on every frame. Packets MUST NOT contain Q = 255 and Length = 0.
| timestamp, | ||
| parameters: Some(VideoParameters { | ||
| pixel_dimensions: (width as u32, height as u32), | ||
| rfc6381_codec: "".to_string(), // RFC 6381 is not applicable to MJPEG |
There was a problem hiding this comment.
We could switch this to an Option; I don't think a version bump for an API change would be a big deal.
There was a problem hiding this comment.
Would you like me to make that change in this PR?
There was a problem hiding this comment.
Would you like me to make that change in this PR?
Eh, I think it's pretty harmless to have the blank string too, so I'll just make a note to do it next time we have a version bump.
|
Sorry for the late reply as well! Apparently my github notifications were turned off. I've made a few commits to fix the issues you pointed out. |
Good point. It would probably just make the API more complicated. The idea to change |
|
What would you say to extending the example client to exercise this in some fashion—like writing a sequence of |
|
That sounds like a good idea. I'll try to get time to do that soon! |
|
I've added a test and an example. The example works pretty much like the mp4 one except that you specify an output directory to the JPEGs. |
|
Finally getting through all the open PRs. Sorry yours has been waiting so long. Very nice work.
Thank you! I don't think any of my cameras do MJPEG so I did a bit of hunting to come up with something to test against. I ended up with this Rube Goldberg machine:
...and then found gstreamer's
In the process I discovered what seems to be code for muxing jpegs to |
Hi!
I've added MJPEG support to the crate based on RFC 2435.
It didn't fit in perfectly with the existing architecture (would maybe be nicer to add a new
ImageParametersstruct), but I think it's good enough?I've verified that it works on two camera models from FlexWatch and Parantek. It also works with the GStreamer RTSP server.
I fuzzed it over the night and it didn't find anything.
Appreciate feedback and ideas for improvements!