Make the code setting uv_angle match the AV1 spec

The following snippet is found in the AV1 spec in intra_frame_mode_info( ) and intra_block_mode_info( ):

        if ( UVMode == UV_CFL_PRED ) {
            read_cfl_alphas( )
        }
        intra_angle_info_uv( )

Note that there is no 'else' after the if statement. Therefore, in decode_b(), the code that sets b->uv_angle should not be preceded by an 'else'.

added 1 commit

• 3612623a - Make the code setting uv_angle match the AV1 spec

Compare with previous version

The current code uses else because those code paths are mutually exclusive.

The first branch is taken if b->uv_mode == CFL_PRED, which means that b->uv_mode >= VERT_PRED && b->uv_mode <= VERT_LEFT_PRED is false.

Or in spec-terms, UVMode == UV_CFL_PRED means !is_directional_mode( UVMode )

Henrik,

Thank you for the reply. A problem with the current code is that b->uv_angle is left uninitialized if b->uv_mode == CFL_PRED. Then, the following code in src/recon_tmpl.c may use b->uv_angle when b->uv_mode == CFL_PRED:

                        if ((b->uv_mode == CFL_PRED && b->cfl_alpha[pl]) ||
                            b->pal_sz[1])
                        {
                            goto skip_uv_pred;
                        }

                        int angle = b->uv_angle;

Our fuzzer showed that it is possible to reach the int angle = b->uv_angle; statement with b->uv_mode == CFL_PRED.

I can change the merge request to first set b->uv_angle to 0 and then do an if-else-if as follows:

            b->uv_angle = 0;
            if (b->uv_mode == CFL_PRED) {
                ...
            } else if (b_dim[2] + b_dim[3] >= 2 && b->uv_mode >= VERT_PRED &&
                       b->uv_mode <= VERT_LEFT_PRED)
            {
                uint16_t *const acdf = ts->cdf.m.angle_delta[b->uv_mode - VERT_PRED];
                const int angle = dav1d_msac_decode_symbol_adapt8(&ts->msac, acdf, 6);
                b->uv_angle = angle - 3;
            }

Would you prefer that?

Zeroing b->uv_angle before the branch to fix the use of an uninitialized variable is fine, yeah.

You should also rebase before pushing by the way, as your branch is currently several hundred commits behind master.

Thank you for pointing that out. I will redo the merge request.

closed