Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gen9 H.264 encoder uses mb_rate_control incorrectly #166

Closed
fhvwy opened this issue May 14, 2017 · 1 comment · Fixed by #167
Closed

gen9 H.264 encoder uses mb_rate_control incorrectly #166

fhvwy opened this issue May 14, 2017 · 1 comment · Fixed by #167
Labels
bug

Comments

@fhvwy
Copy link
Contributor

fhvwy commented May 14, 2017

The va.h header says (va.h:1266):

            unsigned int mb_rate_control : 4; /* Control VA_RC_MB 0: default, 1: enable, 2: disable, other: reserved*/

The H.264 encoder then does (gen9_avc_encoder.c:427/435):

        generic_state->mb_brc_enabled = encoder_context->brc.mb_rate_control[0];

and then treats mb_brc_enabled as a boolean thereafter. This is wrong for mb_rate_control == 2, which should disable it.

(For comparison, the H.265 encoder is correct (gen9_hevc_encoder.c:1702):

            if (brc_method == HEVC_BRC_VCM ||
                encoder_context->brc.mb_rate_control[0] == 0)
                priv_state->lcu_brc_enabled = (priv_state->tu_mode == HEVC_TU_BEST_QUALITY);
            else if (brc_method == HEVC_BRC_ICQ ||
                     encoder_context->brc.mb_rate_control[0] == 1)
                priv_state->lcu_brc_enabled = 1;
            else
                priv_state->lcu_brc_enabled = 0;

That is: given 0, use a default dependent on the configuration; given 1, enable; given 2, disable. (Ignoring the ICQ case because it isn't actually enabled in any configuration.))
@QuPengfei
Copy link

@fhvwy agree. it should be fixed.

fhvwy added a commit to fhvwy/intel-vaapi-driver that referenced this issue May 15, 2017
@fhvwy
Gen9 H.264 encoders: Fix condition for enabling MB rate control
1b2a648 
As documented in va.h, it should not be enabled when the user passes
the value 2 here.  This now matches the behaviour of the H.265
encoder.

Fixes intel#166.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy fhvwy mentioned this issue May 15, 2017
Merged
@xhaihao xhaihao added the bug label May 16, 2017
xhaihao pushed a commit that referenced this issue May 16, 2017
@fhvwy @xhaihao
Gen9 H.264 encoders: Fix condition for enabling MB rate control
0dd6ba4 
As documented in va.h, it should not be enabled when the user passes
the value 2 here.  This now matches the behaviour of the H.265
encoder.

Fixes #166.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
xhaihao pushed a commit that referenced this issue Jun 12, 2017
@fhvwy @xhaihao
Gen9 H.264 encoders: Fix condition for enabling MB rate control
4301174 
As documented in va.h, it should not be enabled when the user passes
the value 2 here.  This now matches the behaviour of the H.265
encoder.

Fixes #166.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
(cherry picked from commit 0dd6ba4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Gen9 H.264 encoders: Fix condition for enabling MB rate control fhvwy/intel-vaapi-driver
3 participants
@QuPengfei @fhvwy @xhaihao