On 08.06.2019, at 03:08, Peter Ross <pr...@xvid.org> wrote:
> ---
> comments against v4 patch addressed. thanks.
>
> +#if CONFIG_VP4_DECODER
> +static int vp4_get_mb_count(Vp3DecodeContext *s, GetBitContext *gb)
> +{
> + int v = 1;
> + int bits;
> + while ((bits = show_bits(gb, 9)) == 0x1ff && v <
> s->yuv_macroblock_count) {
I know some people prefer it, so leave it in that case.
But I think avoiding an assignment in the while makes the code
sufficiently more readable that it's worth the extra line of code.
Also why not just check the v < limit inside the loop after the v+= and
immediatedly return?
This would allow choosing the error return value,
printing a warning etc if desired, and wouldn't unintentionally (?) run the
body(7) code.
> + memset(s->superblock_coding + i, 2 * bit, current_run);
> + has_partial |= bit == 0;
> + bit ^= 1;
Maybe it would be too obfuscating, but you could flip these last 2 lines around
and have
has_partial |= bit;
...
> + if (current_run)
> + av_log(s->avctx, AV_LOG_ERROR, "Invalid run length\n");
Maybe "Invalid partial block run length"? To distinguish the different run
lengths.
> +/* note: dc_pred points to the current block */
> +static int vp4_dc_pred(const Vp3DecodeContext *s, const VP4Predictor *
> dc_pred, const int * last_dc, int type, int plane)
> +{
> + int count = 0;
> + int dc = 0;
[...]
> + return count == 2 ? dc / count : last_dc[type];
This looks a bit weird. Is dc /count somehow clearer than dc/2 here?
Can dc actually be negative so that dc / 2 is different from dc >> 1?
If not the compiler probably will generate needless extra code here.
>
> + static const int motion_shift[2] = { 1, 2 };
> + static const int subpel_mask[2] = { 1, 3 };
> + x = 8 * bx + motion_x / (1 << motion_shift[!!plane]);
> + y = 8 * by + motion_y / (1 << motion_shift[!!plane]);
> +
> + x_subpel = motion_x & subpel_mask[!!plane];
> + y_subpel = motion_y & subpel_mask[!!plane];
I'd probably go for
// using division instead of shift to correctly handle negative values
int motion_div = plane ? 4 : 2;
int subpel_mask = plane ? 3 : 1;
Instead of the array lookup...
I hope these are indeed my final comments now :)
It looks really good to me as far as I am qualified to comment.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
