Fix some problems. 2016-08-18 21:50 GMT+03:00 Moritz Barsnick <barsn...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
--
Станислав Долганов
0002-FFV1-p-frames.patch
Description: Binary data
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
