[FFmpeg-devel] [GSOC][PATCH 2/4] FFV1 p frames

[Moritz Barsnick]

On Thu, Aug 18, 2016 at 14:49:28 +0300, Станислав Долганов wrote:

> +static int decode_q_branch(FFV1Context *f, int level, int x, int y){
> +    RangeCoder *const c = &f->slice_context[0]->c;
> +    OBMCContext *s = &f->obmc;
> +    const int w= s->b_width << s->block_max_depth;

This whole function breaks ffmpeg style (wrt brackets and whitespace)
throughout. How come style is so different here from the rest of the
patch?

> @@ -409,6 +554,7 @@ static int read_extra_header(FFV1Context *f)
>      ff_build_rac_states(c, 0.05 * (1LL << 32), 256 - 8);
>  
>      f->version = get_symbol(c, state, 0);
> +
>      if (f->version < 2) {

This is still a stray change.

>          if ((ret = read_header(f)) < 0)
>              return ret;
>          f->key_frame_ok = 1;
> +
>      } else {

This is still a stray change.
 
> +        for(plane_index=0; plane_index < f->obmc.nb_planes; plane_index++){
> +            PlaneObmc *pc= &f->obmc.plane[plane_index];
> +            int w= pc->width;
> +            int h= pc->height;
> +
> +            if(!p->key_frame){

Whitespace style.

> @@ -906,6 +1153,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
>      if (f->last_picture.f)
>          ff_thread_release_buffer(avctx, &f->last_picture);
>      f->cur = NULL;
> +
>      if ((ret = av_frame_ref(data, f->picture.f)) < 0)
>          return ret;

Yet another stray change.

> @@ -917,15 +1165,24 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
>  #if HAVE_THREADS
>  static int init_thread_copy(AVCodecContext *avctx)
>  {
> +
>      FFV1Context *f = avctx->priv_data;
>      int i, ret;

Ditto.


> -        ThreadFrame picture = fdst->picture, last_picture = fdst->last_picture;
> +        ThreadFrame picture = fdst->picture, last_picture = fdst->last_picture, residual = fdst->residual;
> +        uint16_t *c_image_line_buf = fdst->c_image_line_buf, *p_image_line_buf = fdst->p_image_line_buf;

I personally find comma-separated declarations with assignments very
hard to read, but I don't know whether there's a policy on that. (And
you may be mixing declarations and assignments, which will give
warnings.)

> +        for (i = 0; i < MAX_REF_FRAMES; i++)
> +            last_pictures[i] = fdst->obmc.last_pictures[i];

memcpy()? (Not sure.)
This occurs a few times.

> @@ -1003,13 +1322,41 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
>  
>      av_assert1(fdst->max_slice_count == fsrc->max_slice_count);
>  
> -
>      ff_thread_release_buffer(dst, &fdst->picture);

Another stray change.

> +        for(j=0; j<9; j++) {
> +            int is_chroma= !!(j%3);
> +            int h= is_chroma ? AV_CEIL_RSHIFT(fsrc->avctx->height, fsrc->chroma_v_shift) : fsrc->avctx->height;
> +            int ls= fdst->obmc.last_pictures[i]->linesize[j%3];

Whitespace style.

> +    uint8_t state[128 + 32*128];

I saw that same number somewhere above. Could it be defined as a
constant?

> +        rc = &coder->c; state = coder->state;

Putting these on the same line is not necessary.

> +    coder->c.bytestream_start = coder->c.bytestream = coder->buffer; //FIXME end/start? and at the other stoo

Do the FIXMEs need to get fixed before the patch is ready for
inclusion?

> +    if (c->priv_data) {
> +        av_freep(&c->priv_data);

I thought Michael had explained that the NULL check is not necessary?

> +static void put_block_type  (struct ObmcCoderContext *c, int ctx, int type)

Stray whitespace. Are you trying to align the brackets in this group of
functions?

> +    if (!f->key_frame) { //FIXME update_mc

Fix this?

> +        for(plane_index=0; plane_index<FFMIN(f->obmc.nb_planes, 2); plane_index++){
> +            PlaneObmc *p= &f->obmc.plane[plane_index];

Again, whitespace.

> +    const int width= f->avctx->width;
> +    const int height= f->avctx->height;

Whitespace.

> +        for(i=0; i < f->obmc.nb_planes; i++)
> +        {
> +            int hshift= i ? f->chroma_h_shift : 0;
> +            int vshift= i ? f->chroma_v_shift : 0;

Ditto.

> +        for(plane_index=0; plane_index < f->obmc.nb_planes; plane_index++){
> +            PlaneObmc *p= &f->obmc.plane[plane_index];
> +            int w= p->width;
> +            int h= p->height;
> +
> +            if(pic->pict_type == AV_PICTURE_TYPE_I) {

Ditto.

Functionally, I have no understanding of the code though. ;-)

Moritz