fix(encoder): align VAAPI H.264 encoding resolution to 16-pixel bound…#472
Closed
Resurgamz wants to merge 2 commits into
Closed
fix(encoder): align VAAPI H.264 encoding resolution to 16-pixel bound…#472Resurgamz wants to merge 2 commits into
Resurgamz wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Resurgamz 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 |
|
Hi @Resurgamz. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Reviewer's GuideAligns VAAPI H.264 encoding to 16‑pixel macroblock boundaries by tracking aligned codec dimensions, padding NV12 input frames, and updating frame buffers/linesizes so hardware surfaces and container metadata remain consistent while avoiding visual artifacts. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
out_yy,out_uuvv,vaapi_coded_w, andvaapi_coded_hbeing static globals makes the VAAPI path non‑reentrant and potentially unsafe with multiple encoder instances; consider moving these into the encoder/vaapi context instead of using file‑scope state. - Both new allocations for
out_uuvvandout_yyassumemallocsucceeds; it would be safer to check forNULLand fail encoder initialization cleanly rather than proceeding with invalid buffers. - When reinitializing VAAPI (or if
encoder_video_init_vaapican be called multiple times),out_uuvv/out_yyare overwritten without freeing existing buffers first; either free any previous allocations before reassigning or guard against double initialization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `out_yy`, `out_uuvv`, `vaapi_coded_w`, and `vaapi_coded_h` being static globals makes the VAAPI path non‑reentrant and potentially unsafe with multiple encoder instances; consider moving these into the encoder/vaapi context instead of using file‑scope state.
- Both new allocations for `out_uuvv` and `out_yy` assume `malloc` succeeds; it would be safer to check for `NULL` and fail encoder initialization cleanly rather than proceeding with invalid buffers.
- When reinitializing VAAPI (or if `encoder_video_init_vaapi` can be called multiple times), `out_uuvv`/`out_yy` are overwritten without freeing existing buffers first; either free any previous allocations before reassigning or guard against double initialization.
## Individual Comments
### Comment 1
<location path="libcam/libcam_encoder/encoder.c" line_range="2635-2640" />
<code_context>
+ uint8_t *iu = input_frame + size;
+ uint8_t *iv = input_frame + size + size / 4;
+
+ memset(out_yy, 0, aligned_width * aligned_height);
+ for (int row = 0; row < height; row++)
+ memcpy(out_yy + row * aligned_width, input_frame + row * width, width);
+ y_plane = out_yy;
+
+ memset(out_uuvv, 0x80, aligned_width * aligned_height / 2);
+ for (int hh = 0; hh < height / 2; hh++) {
+ for (int ww = 0; ww < width / 2; ww++) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use size_t for buffer size calculations to avoid potential integer overflows.
Here the size passed to `memset` (and earlier `malloc`) is computed as `aligned_width * aligned_height` while both are `int`, so the multiplication can overflow before being widened to `size_t`. Please compute the size in `size_t` (e.g., `size_t buf_size = (size_t)aligned_width * aligned_height;`) to avoid overflow for larger dimensions and to clarify the intent.
Suggested implementation:
```c
int aligned_width = vaapi_coded_w;
int aligned_height = vaapi_coded_h;
if (aligned_width != width || aligned_height != height) {
/* Input is smaller than codec context: build padded NV12 frame.
* Y plane: copy row by row into aligned-width linesize buffer.
* UV plane: interleave U/V with padding rows filled to neutral gray. */
uint8_t *iu = input_frame + size;
uint8_t *iv = input_frame + size + size / 4;
size_t y_plane_size = (size_t)aligned_width * (size_t)aligned_height;
memset(out_yy, 0, y_plane_size);
for (int row = 0; row < height; row++)
memcpy(out_yy + row * aligned_width, input_frame + row * width, width);
y_plane = out_yy;
size_t uv_plane_size = y_plane_size / 2;
memset(out_uuvv, 0x80, uv_plane_size);
for (int hh = 0; hh < height / 2; hh++) {
for (int ww = 0; ww < width / 2; ww++) {
out_uuvv[hh * aligned_width + ww * 2] = iu[hh * width / 2 + ww];
out_uuvv[hh * aligned_width + ww * 2 + 1] = iv[hh * width / 2 + ww];
}
}
} else {
```
To fully address overflow risks, you should also:
1. Update any `malloc`/`calloc` calls that allocate `out_yy` and `out_uuvv` to use `size_t`-based size calculations, e.g. `size_t y_plane_size = (size_t)aligned_width * aligned_height; out_yy = malloc(y_plane_size);`.
2. Similarly, ensure any other buffer size computations involving `aligned_width` and `aligned_height` in this file use `size_t` intermediates before multiplication.
</issue_to_address>
### Comment 2
<location path="libcam/libcam_encoder/encoder.c" line_range="2058" />
<code_context>
struct SwsContext *swsContext = NULL;
AVFrame *hw_frame = NULL;
uint8_t *out_uuvv = NULL;
+uint8_t *out_yy = NULL;
+static int vaapi_coded_w = 0;
+static int vaapi_coded_h = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving VAAPI-specific buffers and dimensions into the encoder context and encapsulating the NV12 alignment logic in a helper to simplify the encode path and reduce hidden state.
You can keep the new behavior while reducing complexity and hidden state by:
1. **Move VAAPI-specific state into the encoder context**
Instead of `static` globals (`vaapi_coded_w`, `vaapi_coded_h`, `out_yy`, `out_uuvv`), put them into `encoder_video_context_t` (or a VAAPI sub-struct), so they’re clearly tied to an instance and lifetime is explicit.
```c
// encoder_video_context_t (example)
typedef struct encoder_video_context {
...
int vaapi_coded_w;
int vaapi_coded_h;
uint8_t *vaapi_out_yy;
uint8_t *vaapi_out_uuvv;
...
} encoder_video_context_t;
```
Then, in init:
```c
enc_video_ctx->vaapi_coded_w = video_codec_data->codec_context->width;
enc_video_ctx->vaapi_coded_h = video_codec_data->codec_context->height;
enc_video_ctx->vaapi_out_uuvv = malloc(enc_video_ctx->vaapi_coded_w *
enc_video_ctx->vaapi_coded_h / 2);
enc_video_ctx->vaapi_out_yy = malloc(enc_video_ctx->vaapi_coded_w *
enc_video_ctx->vaapi_coded_h);
```
And in `vaapi_over()`:
```c
free(enc_video_ctx->vaapi_out_uuvv);
free(enc_video_ctx->vaapi_out_yy);
```
This removes global hidden dependencies from `encoder_encode_video_vaapi` and keeps all VAAPI buffer ownership in one place.
2. **Extract the alignment / padding into a helper**
The encode hot path becomes easier to read if you move the branchy alignment + memcpy/memset logic into a small helper that returns Y/UV pointers and linesizes.
```c
static void prepare_nv12_vaapi_frame(uint8_t *input_frame,
int width, int height, int size,
encoder_video_context_t *enc_video_ctx,
uint8_t **y_plane, uint8_t **uv_plane,
int *aligned_width, int *aligned_height)
{
int aw = enc_video_ctx->vaapi_coded_w;
int ah = enc_video_ctx->vaapi_coded_h;
*aligned_width = aw;
*aligned_height = ah;
if (aw != width || ah != height) {
uint8_t *iu = input_frame + size;
uint8_t *iv = input_frame + size + size / 4;
memset(enc_video_ctx->vaapi_out_yy, 0, aw * ah);
for (int row = 0; row < height; row++)
memcpy(enc_video_ctx->vaapi_out_yy + row * aw,
input_frame + row * width, width);
memset(enc_video_ctx->vaapi_out_uuvv, 0x80, aw * ah / 2);
for (int hh = 0; hh < height / 2; hh++) {
for (int ww = 0; ww < width / 2; ww++) {
enc_video_ctx->vaapi_out_uuvv[hh * aw + ww * 2] =
iu[hh * width / 2 + ww];
enc_video_ctx->vaapi_out_uuvv[hh * aw + ww * 2 + 1] =
iv[hh * width / 2 + ww];
}
}
*y_plane = enc_video_ctx->vaapi_out_yy;
*uv_plane = enc_video_ctx->vaapi_out_uuvv;
return;
}
uint8_t *uv = enc_video_ctx->vaapi_out_uuvv;
uint8_t *iu = input_frame + size;
uint8_t *iv = input_frame + size + size / 4;
for (int hh = 0; hh < height; hh++) {
for (int ww = 0; ww < width / 4; ww++) {
*uv++ = *iu++;
*uv++ = *iv++;
}
}
*y_plane = input_frame;
*uv_plane = enc_video_ctx->vaapi_out_uuvv;
}
```
Then your encode function is simplified:
```c
uint8_t *y_plane, *uv_plane;
int aligned_width, aligned_height;
prepare_nv12_vaapi_frame(input_frame, width, height, size,
enc_video_ctx,
&y_plane, &uv_plane,
&aligned_width, &aligned_height);
video_codec_data->frame->format = AV_PIX_FMT_NV12;
video_codec_data->frame->width = aligned_width;
video_codec_data->frame->height = aligned_height;
video_codec_data->frame->data[0] = y_plane;
video_codec_data->frame->data[1] = uv_plane;
video_codec_data->frame->linesize[0] = aligned_width;
video_codec_data->frame->linesize[1] = aligned_width;
video_codec_data->frame->linesize[2] = 0;
```
This keeps `encoder_encode_video_vaapi` focused on “wire up FFmpeg frame fields” while encapsulating all the alignment/padding details and using a single, consistent source for the coded dimensions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…aries - Align VAAPI codec_context dimensions to 16 pixels for H.264 macroblock requirements - Add padded NV12 frame construction when input resolution is not 16-aligned - Add out_yy buffer for Y plane row-by-row copy with aligned linesize - Add vaapi_coded_w/vaapi_coded_h static variables to track aligned dimensions - Fill UV padding area with 0x80 (neutral gray) to avoid visual artifacts - Only apply alignment for H.264 via VAAPI; software encoders unchanged 修复(encoder): 对齐 VAAPI H.264 编码分辨率至 16 像素边界 - 将 VAAPI codec_context 尺寸按 16 像素对齐，满足 H.264 宏块要求 - 输入分辨率非 16 对齐时，构造带填充的 NV12 帧 - 新增 out_yy 缓冲区，按对齐行宽逐行拷贝 Y 平面数据 - 新增 vaapi_coded_w/vaapi_coded_h 静态变量记录对齐后的编码尺寸 - UV 填充区域填充 0x80（中性灰），避免视觉伪影 - 仅对 VAAPI H.264 应用对齐，软件编码路径不做修改 Log: 修复 VAAPI 硬件编码 H.264 时非 16 对齐分辨率导致视频无法播放的问题，通过将编码分辨率对齐至 16 像素边界并正确填充 NV12 帧数据 Bug: https://pms.uniontech.com/bug-view-345769.html
- 切换回拍照模式时所有4个按钮的m_iconOpacity均通过 setIconOpacity(1)正确恢复 - 与setIconOpacity(1)的行为对齐，统一恢复所有按钮状态 Log: 修复拍照/录像状态切换后m_delayUnfoldBtn背景透明度异常的问题 Bug: https://pms.uniontech.com/bug-view-196565.html
deepin pr auto review你好！我是CodeGeeX。我已经仔细审查了你提供的 总体来说，这次修改的方向非常正确，解决了VAAPI编码器对H.264宏块16x16对齐的硬性要求，并消除了全局变量，提高了代码的模块化和线程安全性。 但在语法逻辑、代码质量、性能和安全方面，我发现了一些需要改进的隐患。以下是详细的审查意见： 一、 语法与逻辑1. 2.
3. 二、 代码性能1. for (int hh = 0; hh < height / 2; hh++) {
for (int ww = 0; ww < width / 2; ww++) {
enc_video_ctx->vaapi_out_uuvv[hh * aw + ww * 2] = iu[...];
enc_video_ctx->vaapi_out_uuvv[hh * aw + ww * 2 + 1] = iv[...];
}
}问题：视频编码是性能敏感路径，这种逐字节拷贝的方式没有利用到 CPU 的向量指令（SIMD），且循环开销巨大，会导致编码帧率大幅下降。 三、 代码安全1. video_codec_data->codec_context->width = ((encoder_ctx->video_width + 15) / 16) * 16;问题：如果 size_t y_plane_size = (size_t)enc_video_ctx->vaapi_coded_w * (size_t)enc_video_ctx->vaapi_coded_h;
enc_video_ctx->vaapi_out_yy = (uint8_t *)malloc(y_plane_size);如果宽高异常， 2. 四、 代码质量与可读性1. 遗留的全局变量 2. UI 相关的 m_flashlightUnfoldBtn->setIconOpacity(1);问题： 🌟 改进后的代码示例针对核心的 // 修复1：参数校验与更安全的 NV12 准备逻辑
static void prepare_nv12_vaapi_frame(uint8_t *input_frame,
int width, int height, int size,
encoder_video_context_t *enc_video_ctx,
uint8_t **y_plane, uint8_t **uv_plane,
int *aligned_width, int *aligned_height)
{
// 防御性校验
if (!input_frame || !enc_video_ctx || !enc_video_ctx->vaapi_out_yy || !enc_video_ctx->vaapi_out_uuvv) {
*y_plane = NULL;
*uv_plane = NULL;
return;
}
int aw = enc_video_ctx->vaapi_coded_w;
int ah = enc_video_ctx->vaapi_coded_h;
*aligned_width = aw;
*aligned_height = ah;
const uint8_t *iu = input_frame + size;
const uint8_t *iv = input_frame + size + size / 4;
if (aw != width || ah != height) {
/* 需要对齐的分支 */
size_t y_plane_size = (size_t)aw * (size_t)ah;
size_t uv_plane_size = y_plane_size / 2;
memset(enc_video_ctx->vaapi_out_yy, 0, y_plane_size);
for (int row = 0; row < height; row++)
memcpy(enc_video_ctx->vaapi_out_yy + row * aw,
input_frame + row * width, width);
memset(enc_video_ctx->vaapi_out_uuvv, 0x80, uv_plane_size); // UV填充中性色
// 性能优化 & 逻辑修复：统一使用 width/2 步进，避免非4倍数宽度丢像素
int uv_width = width / 2;
int uv_height = height / 2;
for (int hh = 0; hh < uv_height; hh++) {
uint8_t *out_uv_line = enc_video_ctx->vaapi_out_uuvv + hh * aw;
const uint8_t *in_u_line = iu + hh * uv_width;
const uint8_t *in_v_line = iv + hh * uv_width;
for (int ww = 0; ww < uv_width; ww++) {
out_uv_line[ww * 2] = in_u_line[ww];
out_uv_line[ww * 2 + 1] = in_v_line[ww];
}
}
*y_plane = enc_video_ctx->vaapi_out_yy;
*uv_plane = enc_video_ctx->vaapi_out_uuvv;
} else {
/* 不需要对齐的分支 */
uint8_t *uv = enc_video_ctx->vaapi_out_uuvv;
// 逻辑修复：统一使用 width/2 步进，与对齐分支保持一致，修复非4倍数宽度的问题
int uv_width = width / 2;
int uv_height = height / 2; // 注意：NV12的UV高度是Y的一半
for (int hh = 0; hh < uv_height; hh++) {
const uint8_t *in_u_line = iu + hh * uv_width;
const uint8_t *in_v_line = iv + hh * uv_width;
for (int ww = 0; ww < uv_width; ww++) {
*uv++ = in_u_line[ww];
*uv++ = in_v_line[ww];
}
}
*y_plane = input_frame;
*uv_plane = enc_video_ctx->vaapi_out_uuvv;
}
}
// 修复2：初始化时的参数校验与安全退出机制
static encoder_video_context_t *encoder_video_init_vaapi(encoder_context_t *encoder_ctx) {
// ... 前面的代码 ...
/* 增加宽高校验，防止异常值导致分配0字节或溢出 */
if (encoder_ctx->video_width <= 0 || encoder_ctx->video_height <= 0) {
fprintf(stderr, "Invalid video dimensions: %dx%d\n", encoder_ctx->video_width, encoder_ctx->video_height);
goto fail; // 假设函数末尾有 fail 标签释放资源
}
if (video_defaults->codec_id == AV_CODEC_ID_H264) {
video_codec_data->codec_context->width = ((encoder_ctx->video_width + 15) / 16) * 16;
video_codec_data->codec_context->height = ((encoder_ctx->video_height + 15) / 16) * 16;
} else {
video_codec_data->codec_context->width = encoder_ctx->video_width;
video_codec_data->codec_context->height = encoder_ctx->video_height;
}
// ... 中间的代码 ...
enc_video_ctx->vaapi_coded_w = video_codec_data->codec_context->width;
enc_video_ctx->vaapi_coded_h = video_codec_data->codec_context->height;
{
size_t y_plane_size = (size_t)enc_video_ctx->vaapi_coded_w * (size_t)enc_video_ctx->vaapi_coded_h;
size_t uv_plane_size = y_plane_size / 2;
enc_video_ctx->vaapi_out_yy = (uint8_t *)malloc(y_plane_size);
enc_video_ctx->vaapi_out_uuvv = (uint8_t *)malloc(uv_plane_size);
if (!enc_video_ctx->vaapi_out_yy || !enc_video_ctx->vaapi_out_uuvv) {
fprintf(stderr, "Failed to allocate VAAPI working buffers.\n");
free(enc_video_ctx->vaapi_out_yy);
free(enc_video_ctx->vaapi_out_uuvv);
enc_video_ctx->vaapi_out_yy = NULL;
enc_video_ctx->vaapi_out_uuvv = NULL;
is_vaapi = HW_VAAPI_FAIL29;
// 严重修复：不能直接 return NULL，必须跳转到 fail 标签释放已分配的 FFmpeg 上下文
goto fail;
}
}
// ... 后面的代码 ...
}希望这些审查意见对你有所帮助！如果关于 FFmpeg 的 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…aries
修复(encoder): 对齐 VAAPI H.264 编码分辨率至 16 像素边界
Log: 修复 VAAPI 硬件编码 H.264 时非 16 对齐分辨率导致视频无法播放的问题，通过将编码分辨率对齐至 16 像素边界并正确填充 NV12 帧数据
Bug: https://pms.uniontech.com/bug-view-345769.html
Summary by Sourcery
Align VAAPI H.264 encoding to 16-pixel macroblock boundaries and pad input frames so non-16-aligned resolutions can be encoded correctly.
Bug Fixes:
Enhancements: