Added av_device_format parameter for MJPEG #269
Conversation
There was a problem hiding this comment.
@boitumeloruf overally really good PR! I like the idea. Can I ask you to think about maybe a way we can keep the ffmpeg stuff isolated from the rest of the package? That way we have clear separation between ffmpeg vs v4l2 vs anything else
include/usb_cam/usb_cam.hpp
Outdated
| /// @return pixel format structure corresponding to a given name | ||
| inline std::shared_ptr<pixel_format_base> set_pixel_format_from_string(const std::string & str) | ||
| inline std::shared_ptr<pixel_format_base> set_pixel_format_from_string(const std::string & pixFmtStr, | ||
| const std::string & avDevFmtStr) |
There was a problem hiding this comment.
@boitumeloruf so I've tried really hard to keep the ffmpeg stuff separate from the rest of this base library. Right now the only place that has any ffmpeg stuff is in the mjpeg format file.
I dont know if I'd be OK with adding reference to it here.
Is there any other way we could get this av device format string to the conversion class?
There was a problem hiding this comment.
One idea would be to rename the function into just set_pixel_formatand instead of passing the pixel format string to the function, one could pass the full parameter list in the form of the parameter_t struct to the function. In this way, one would have full access to all parameters, allowing one to pass the necessary parameters to the pixel_format class.
There was a problem hiding this comment.
@boitumeloruf good suggestion! Could you implement that here for this PR?
include/usb_cam/usb_cam.hpp
Outdated
| } | ||
|
|
||
| return m_image.pixel_format; | ||
| } | ||
|
|
||
| inline AVPixelFormat get_av_pixel_format_from_string (const std::string& str) |
There was a problem hiding this comment.
@boitumeloruf ideally we keep the AV* ffmpeg stuff out of this file if we can. Can we move this function to somewhere else?
There was a problem hiding this comment.
Well, we could move this function to the av_picel_format_herlper.hpp header file. In this way, all the libavcodec specific code is in one helper file.
There was a problem hiding this comment.
@boitumeloruf I think thats fine and put that new file within the formats directory 👍🏼
|
Hi @flynneva, I have refactored to code as suggested. Ready to be reviewed. |
|
@boitumeloruf do you think you could check out why the CI jobs are failing for this PR? Otherwise I think I'd approve it |
The jobs failed due to formatting errors detected by cppcheck and uncrustify. I have fixed those now. |
When accessing the MJPEG stream of the camera and decoding it into RGB (or other pixel format), the AV-Decoder needs a pixel format of the device. To this end, this was fixed in the case of mjpeg2rgb. With this pull request I suggest to introduce another config parameter with which the device pixel format for the AV-Decoder can be specified. This can either be done by the full string of the enum name or just the format-specific suffix as done in the example YAML files.