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

[Станислав Долганов]

Fix some problems.

2016-08-18 21:50 GMT+03:00 Moritz Barsnick <barsnick at gmx.net>:

> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>



-- 
Станислав Долганов
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-FFV1-p-frames.patch
Type: application/octet-stream
Size: 42883 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160819/1af20291/attachment.obj>