fix: use real MJPG decoded size instead of negotiated resolution - #520
Conversation
Reviewer's GuideAdjust MJPG decoding to use the actual decoded frame size from FFmpeg instead of the negotiated resolution, and propagate that real size through buffer management and frame metadata so frames are copied, resized, and trimmed safely. Sequence diagram for MJPG decode using real decoded sizesequenceDiagram
participant FrameDecoder as decode_v4l2_frame
participant JpegDecoder as jpeg_decoder
participant FFmpeg
participant FrameBuffer as v4l2_frame_buff_t
FrameDecoder->>JpegDecoder: jpeg_decode(out_buf, in_buf, size)
JpegDecoder->>FFmpeg: m_av_image_get_buffer_size(format, dec_w, dec_h, 1)
FFmpeg-->>JpegDecoder: buffer_size
JpegDecoder->>JpegDecoder: realloc(tmp_frame, buffer_size)
JpegDecoder->>FFmpeg: m_av_image_copy_to_buffer(tmp_frame, pic_size, data, linesize, format, dec_w, dec_h, 1)
JpegDecoder->>JpegDecoder: update jpeg_ctx.width, jpeg_ctx.height
JpegDecoder-->>FrameDecoder: ret
FrameDecoder->>JpegDecoder: jpeg_get_decoded_size(out_w, out_h)
JpegDecoder-->>FrameDecoder: real_w, real_h
FrameDecoder->>FrameDecoder: adjust frame.width, frame.height
FrameDecoder->>FrameBuffer: realloc(yuv_frame, need)
FrameDecoder->>FrameBuffer: update yuv_frame_max_size
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In both jpeg_decode paths where you compute
needandrealloc(jpeg_ctx->tmp_frame, need), ifreallocfails you still callm_av_image_copy_to_bufferwithjpeg_ctx->pic_sizesmaller thanneed, which risks a buffer overrun; consider bailing out or falling back whenneed > jpeg_ctx->pic_sizeandreallocreturns NULL. - When computing
size_t need = (size_t)real_w * (size_t)real_h * 3 / 2indecode_v4l2_frame, it may be safer to guard against integer overflow for very large resolutions (e.g., by checking the multiplication operands before performing the multiplication).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both jpeg_decode paths where you compute `need` and `realloc(jpeg_ctx->tmp_frame, need)`, if `realloc` fails you still call `m_av_image_copy_to_buffer` with `jpeg_ctx->pic_size` smaller than `need`, which risks a buffer overrun; consider bailing out or falling back when `need > jpeg_ctx->pic_size` and `realloc` returns NULL.
- When computing `size_t need = (size_t)real_w * (size_t)real_h * 3 / 2` in `decode_v4l2_frame`, it may be safer to guard against integer overflow for very large resolutions (e.g., by checking the multiplication operands before performing the multiplication).
## Individual Comments
### Comment 1
<location path="libcam/libcam_v4l2core/jpeg_decoder.c" line_range="1586-1585" />
<code_context>
+ {
+ int dec_w = sw_frame->width;
+ int dec_h = sw_frame->height;
+ int need = getAvutil()->m_av_image_get_buffer_size(sw_frame->format, dec_w, dec_h, 1);
+ if (need > 0 && (size_t)need > (size_t)jpeg_ctx->pic_size) {
+ uint8_t *p = realloc(jpeg_ctx->tmp_frame, (size_t)need);
+ if (p) { jpeg_ctx->tmp_frame = p; jpeg_ctx->pic_size = need; }
+ }
+ jpeg_ctx->width = dec_w;
+ jpeg_ctx->height = dec_h;
+ getAvutil()->m_av_image_copy_to_buffer(jpeg_ctx->tmp_frame, jpeg_ctx->pic_size,
+ (const uint8_t * const*) sw_frame->data, sw_frame->linesize,
+ sw_frame->format, dec_w, dec_h, 1);
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential buffer overflow if realloc fails but `need` exceeds `jpeg_ctx->pic_size`.
If `need > jpeg_ctx->pic_size` and `realloc` returns NULL, we still call `av_image_copy_to_buffer` with `dec_w/dec_h` but a buffer sized only for the old `pic_size`, so the copy can overflow. We should either return an error when `realloc` fails in this case, or constrain the copy to dimensions that fit within the existing `pic_size` to avoid undefined behavior.
</issue_to_address>
### Comment 2
<location path="libcam/libcam_v4l2core/frame_decoder.c" line_range="850-853" />
<code_context>
+
+ if (real_w > 0 && real_h > 0)
+ {
+ size_t need = (size_t)real_w * (size_t)real_h * 3 / 2;
+ if (need > 0 && need != frame->yuv_frame_max_size)
+ {
+ uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
+ if (p)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against size_t overflow when computing `need` and clarify trimming behavior.
The expression `(size_t)real_w * (size_t)real_h * 3 / 2` can overflow `size_t` if a malformed stream reports very large dimensions, yielding a too-small `need` passed to `realloc`. Please add a bound check (e.g., max width/height or verifying the product stays below a safe limit) and treat out-of-range sizes as an error. Also consider only shrinking when `need < yuv_frame_max_size` and growing when `need > yuv_frame_max_size` to avoid unnecessary reallocs if other code adjusts `yuv_frame_max_size` for other formats.
Suggested implementation:
```c
if (real_w > 0 && real_h > 0)
{
size_t pixels;
size_t need;
/* guard against overflow in pixels = real_w * real_h */
if ((size_t)real_w > 0 &&
(size_t)real_h > 0 &&
(size_t)real_w > SIZE_MAX / (size_t)real_h)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) decoded frame size %dx%d too large, rejecting\n",
real_w, real_h);
return -1;
}
pixels = (size_t)real_w * (size_t)real_h;
/* guard against overflow in need = pixels * 3 / 2 */
if (pixels > SIZE_MAX / 3)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) decoded frame size %dx%d too large, rejecting\n",
real_w, real_h);
return -1;
}
need = pixels * 3 / 2;
if (need == 0)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) computed zero-sized YUV buffer for %dx%d\n",
real_w, real_h);
return -1;
}
/* Only reallocate when we actually need to grow or shrink */
if (need > frame->yuv_frame_max_size)
{
uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
if (!p)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) realloc to %zu bytes failed (grow)\n",
need);
return -1;
}
frame->yuv_frame = p;
frame->yuv_frame_max_size = need;
}
else if (need < frame->yuv_frame_max_size)
{
uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
if (!p)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) realloc to %zu bytes failed (shrink)\n",
need);
return -1;
}
frame->yuv_frame = p;
frame->yuv_frame_max_size = need;
}
```
1. Ensure `SIZE_MAX` is available by including `<limits.h>` at the top of `libcam/libcam_v4l2core/frame_decoder.c` if it is not already included.
2. The `return -1;` error paths assume this code is inside a function that returns an `int`-like status code; if this function has a different return type or uses `goto`-based error handling, replace the `return -1;` with the appropriate error handling (e.g., `goto error;`).
3. If shrinking the buffer is not desired in this code path (to preserve a larger `yuv_frame_max_size` set for other formats), you can remove the `else if (need < frame->yuv_frame_max_size)` block, leaving only the `need > frame->yuv_frame_max_size` grow case.
</issue_to_address>
### Comment 3
<location path="libcam/libcam_v4l2core/jpeg_decoder.c" line_range="1583" />
<code_context>
if(verbosity > 3)
fprintf(stderr, "V4L2_CORE: (jpeg decoder) decode frame of size %i\n", ret);
+ if (ret >= 0)
+ {
+ int real_w = 0;
+ int real_h = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the duplicated buffer sizing and jpeg_ctx width/height update logic into a shared helper to simplify both decode paths and keep them in sync.
You can reduce the new complexity by factoring the duplicated “ensure buffer and update size” logic into a small helper and reusing it in both call sites.
For example, near the top of this file (or as `static` in the same translation unit):
```c
static void jpeg_ensure_tmp_frame_capacity(enum AVPixelFormat fmt,
int dec_w, int dec_h)
{
int need = getAvutil()->m_av_image_get_buffer_size(fmt, dec_w, dec_h, 1);
if (need > 0 && (size_t)need > (size_t)jpeg_ctx->pic_size) {
uint8_t *p = realloc(jpeg_ctx->tmp_frame, (size_t)need);
if (p) {
jpeg_ctx->tmp_frame = p;
jpeg_ctx->pic_size = need;
}
/* if realloc fails, keep old buffer/size; behavior unchanged */
}
jpeg_ctx->width = dec_w;
jpeg_ctx->height = dec_h;
}
```
Then both blocks shrink to the buffer copy plus the helper call, keeping behavior identical:
**NV12 path:**
```c
{
int dec_w = sw_frame->width;
int dec_h = sw_frame->height;
jpeg_ensure_tmp_frame_capacity(sw_frame->format, dec_w, dec_h);
getAvutil()->m_av_image_copy_to_buffer(
jpeg_ctx->tmp_frame, jpeg_ctx->pic_size,
(const uint8_t * const*)sw_frame->data, sw_frame->linesize,
sw_frame->format, dec_w, dec_h, 1
);
}
if (sw_frame->format == AV_PIX_FMT_NV12) {
nv12_to_yu12(out_buf, jpeg_ctx->tmp_frame, jpeg_ctx->width, jpeg_ctx->height);
...
}
```
**Non-NV12 path:**
```c
{
int dec_w = codec_data->picture->width;
int dec_h = codec_data->picture->height;
enum AVPixelFormat dec_fmt = codec_data->picture->format;
jpeg_ensure_tmp_frame_capacity(dec_fmt, dec_w, dec_h);
getAvutil()->m_av_image_copy_to_buffer(
jpeg_ctx->tmp_frame, jpeg_ctx->pic_size,
(const uint8_t * const*)codec_data->picture->data,
codec_data->picture->linesize,
dec_fmt, dec_w, dec_h, 1
);
}
```
This keeps all the new dynamic sizing behavior (including `jpeg_ctx->width/height` updates) but centralizes the logic so future changes only touch one place and both paths stay consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| size_t need = (size_t)real_w * (size_t)real_h * 3 / 2; | ||
| if (need > 0 && need != frame->yuv_frame_max_size) | ||
| { | ||
| uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need); |
There was a problem hiding this comment.
suggestion (bug_risk): Guard against size_t overflow when computing need and clarify trimming behavior.
The expression (size_t)real_w * (size_t)real_h * 3 / 2 can overflow size_t if a malformed stream reports very large dimensions, yielding a too-small need passed to realloc. Please add a bound check (e.g., max width/height or verifying the product stays below a safe limit) and treat out-of-range sizes as an error. Also consider only shrinking when need < yuv_frame_max_size and growing when need > yuv_frame_max_size to avoid unnecessary reallocs if other code adjusts yuv_frame_max_size for other formats.
Suggested implementation:
if (real_w > 0 && real_h > 0)
{
size_t pixels;
size_t need;
/* guard against overflow in pixels = real_w * real_h */
if ((size_t)real_w > 0 &&
(size_t)real_h > 0 &&
(size_t)real_w > SIZE_MAX / (size_t)real_h)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) decoded frame size %dx%d too large, rejecting\n",
real_w, real_h);
return -1;
}
pixels = (size_t)real_w * (size_t)real_h;
/* guard against overflow in need = pixels * 3 / 2 */
if (pixels > SIZE_MAX / 3)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) decoded frame size %dx%d too large, rejecting\n",
real_w, real_h);
return -1;
}
need = pixels * 3 / 2;
if (need == 0)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) computed zero-sized YUV buffer for %dx%d\n",
real_w, real_h);
return -1;
}
/* Only reallocate when we actually need to grow or shrink */
if (need > frame->yuv_frame_max_size)
{
uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
if (!p)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) realloc to %zu bytes failed (grow)\n",
need);
return -1;
}
frame->yuv_frame = p;
frame->yuv_frame_max_size = need;
}
else if (need < frame->yuv_frame_max_size)
{
uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
if (!p)
{
if (verbosity > 0)
fprintf(stderr,
"V4L2_CORE: (jpeg decoder) realloc to %zu bytes failed (shrink)\n",
need);
return -1;
}
frame->yuv_frame = p;
frame->yuv_frame_max_size = need;
}- Ensure
SIZE_MAXis available by including<limits.h>at the top oflibcam/libcam_v4l2core/frame_decoder.cif it is not already included. - The
return -1;error paths assume this code is inside a function that returns anint-like status code; if this function has a different return type or usesgoto-based error handling, replace thereturn -1;with the appropriate error handling (e.g.,goto error;). - If shrinking the buffer is not desired in this code path (to preserve a larger
yuv_frame_max_sizeset for other formats), you can remove theelse if (need < frame->yuv_frame_max_size)block, leaving only theneed > frame->yuv_frame_max_sizegrow case.
| getAvutil()->m_av_image_copy_to_buffer(jpeg_ctx->tmp_frame, jpeg_ctx->pic_size, | ||
| (const uint8_t * const*) sw_frame->data, sw_frame->linesize, | ||
| sw_frame->format, jpeg_ctx->width, jpeg_ctx->height, 1); | ||
| { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the duplicated buffer sizing and jpeg_ctx width/height update logic into a shared helper to simplify both decode paths and keep them in sync.
You can reduce the new complexity by factoring the duplicated “ensure buffer and update size” logic into a small helper and reusing it in both call sites.
For example, near the top of this file (or as static in the same translation unit):
static void jpeg_ensure_tmp_frame_capacity(enum AVPixelFormat fmt,
int dec_w, int dec_h)
{
int need = getAvutil()->m_av_image_get_buffer_size(fmt, dec_w, dec_h, 1);
if (need > 0 && (size_t)need > (size_t)jpeg_ctx->pic_size) {
uint8_t *p = realloc(jpeg_ctx->tmp_frame, (size_t)need);
if (p) {
jpeg_ctx->tmp_frame = p;
jpeg_ctx->pic_size = need;
}
/* if realloc fails, keep old buffer/size; behavior unchanged */
}
jpeg_ctx->width = dec_w;
jpeg_ctx->height = dec_h;
}Then both blocks shrink to the buffer copy plus the helper call, keeping behavior identical:
NV12 path:
{
int dec_w = sw_frame->width;
int dec_h = sw_frame->height;
jpeg_ensure_tmp_frame_capacity(sw_frame->format, dec_w, dec_h);
getAvutil()->m_av_image_copy_to_buffer(
jpeg_ctx->tmp_frame, jpeg_ctx->pic_size,
(const uint8_t * const*)sw_frame->data, sw_frame->linesize,
sw_frame->format, dec_w, dec_h, 1
);
}
if (sw_frame->format == AV_PIX_FMT_NV12) {
nv12_to_yu12(out_buf, jpeg_ctx->tmp_frame, jpeg_ctx->width, jpeg_ctx->height);
...
}Non-NV12 path:
{
int dec_w = codec_data->picture->width;
int dec_h = codec_data->picture->height;
enum AVPixelFormat dec_fmt = codec_data->picture->format;
jpeg_ensure_tmp_frame_capacity(dec_fmt, dec_w, dec_h);
getAvutil()->m_av_image_copy_to_buffer(
jpeg_ctx->tmp_frame, jpeg_ctx->pic_size,
(const uint8_t * const*)codec_data->picture->data,
codec_data->picture->linesize,
dec_fmt, dec_w, dec_h, 1
);
}This keeps all the new dynamic sizing behavior (including jpeg_ctx->width/height updates) but centralizes the logic so future changes only touch one place and both paths stay consistent.
Do not preset decoder width/height before avcodec_open2(); let FFmpeg parse the real size from the JPEG SOF header. Copy decoded frames using the actual sw_frame/picture dimensions and realloc buffers when needed. Add jpeg_get_decoded_size() and trim yuv_frame to the real decoded size. 不在 avcodec_open2() 前预设解码器宽高,改由 FFmpeg 从 JPEG SOF 头解析 真实尺寸。按实际解码帧尺寸拷贝并在需要时 realloc 缓冲。新增 jpeg_get_decoded_size(),并在首帧后将 yuv_frame 收紧到真实解码尺寸。 Log: 修复MJPG解码尺寸与协商分辨率不一致导致的解码失败和画面错位 PMS: BUG-372317 Influence: 修复相机协商与实际分辨率不一致导致的解码失败、画面错位及内存浪费。
f04d3d0 to
ec0c03e
Compare
deepin pr auto review★ 总体评分:90分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 在 frame_decoder.c 的 decode_v4l2_frame 函数中,增加整数溢出保护
if (real_w > 0 && real_h > 0)
{
// 检查乘法溢出
if ((size_t)real_w > (SIZE_MAX / (size_t)real_h) ||
((size_t)real_w * (size_t)real_h) > (SIZE_MAX / 3 * 2)) {
if(verbosity > 0)
fprintf(stderr, "V4L2_CORE: (jpeg decoder) integer overflow for size %dx%d\n", real_w, real_h);
return E_ALLOC_ERR;
}
size_t need = (size_t)real_w * (size_t)real_h * 3 / 2;
if (need > 0 && need != frame->yuv_frame_max_size)
{
uint8_t *p = (uint8_t *)realloc(frame->yuv_frame, need);
if (p)
{
frame->yuv_frame = p;
frame->yuv_frame_max_size = need;
if(verbosity > 0)
fprintf(stderr, "V4L2_CORE: (jpeg decoder) yuv_frame trimmed %dx%d (%zu bytes)\n",
real_w, real_h, need);
}
else
{
if(verbosity > 0)
fprintf(stderr, "V4L2_CORE: (jpeg decoder) realloc failed for yuv_frame, keep old size\n");
return E_ALLOC_ERR;
}
}
} |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: add-uos, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
Do not preset decoder width/height before avcodec_open2(); let FFmpeg parse the real size from the JPEG SOF header. Copy decoded frames using the actual sw_frame/picture dimensions and realloc buffers when needed. Add jpeg_get_decoded_size() and trim yuv_frame to the real decoded size.
不在 avcodec_open2() 前预设解码器宽高,改由 FFmpeg 从 JPEG SOF 头解析 真实尺寸。按实际解码帧尺寸拷贝并在需要时 realloc 缓冲。新增
jpeg_get_decoded_size(),并在首帧后将 yuv_frame 收紧到真实解码尺寸。
Log: 修复MJPG解码尺寸与协商分辨率不一致导致的解码失败和画面错位
PMS: BUG-372317
Influence: 修复相机协商与实际分辨率不一致导致的解码失败、画面错位及内存浪费。
Summary by Sourcery
Use the JPEG’s actual decoded dimensions throughout MJPEG frame processing instead of relying on the negotiated resolution.
New Features:
Bug Fixes:
Enhancements: