[FFmpeg-devel] [PATCH V2] avcodec/libx265: add support for ROI-based encoding

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Derek Buitenhuis
> Sent: Tuesday, January 22, 2019 10:31 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH V2] avcodec/libx265: add support for
> ROI-based encoding
> 
> On 21/01/2019 14:40, Guo, Yejun wrote:
> 
> [...]
> 
> > +        } else {
> > +            // 8x8 block when qg-size is 8, 16*16 block otherwise
> 
> Cosmetic: Comments should be /**/ to match the rest of the file.

ok, and will also change the style of other two comments.

> 
> > +            int mb_size = (ctx->params->rc.qgSize == 8) ? 8 : 16;
> > +            int mbx = (frame->width + mb_size - 1) / mb_size;
> > +            int mby = (frame->height + mb_size - 1) / mb_size;
> > +            int nb_rois;
> > +            AVRegionOfInterest* roi;
> 
> Cosmetic: File style is Type *name;

ok, and will also change another place to float *.

> 
> > +                if (roi->qoffset.den == 0) {
> > +                    av_free(qoffsets);
> > +                    av_log(ctx, AV_LOG_ERROR,
> > + "AVRegionOfInterest.qoffset.den should not be zero.\n");
> 
> Nit: s/should/must/

ok, will also change the wording later.

> > +                for (int y = starty; y < endy; y++) {
> > +                    for (int x = startx; x < endx; x++) {
> > +                        qoffsets[x + y*mbx] = qoffset;
> > +                    }
> > +                }
> 
> Cosmetic: Braces not necessary.

ok, will remove it to match the rest of code.

> 
> > +
> > +                if (roi->self_size == 0) {
> > +                    av_free(qoffsets);
> > +                    av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size
> should be set to sizeof(AVRegionOfInterest).\n");
> > +                    return AVERROR(EINVAL);
> > +                }
> 
> Check should probably be outside loop.

The check have to be within the loop, since 'roi' changes with the loop iteration.

To refine the code, I'll move them near to the check of roi->qoffset.den.

> 
> 
> >  static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> >                                  const AVFrame *pic, int *got_packet)
> > {
> 
> > +    if (x265pic.quantOffsets)
> > +        av_freep(&x265pic.quantOffsets);
> 
> The NULL check is redundant since av_freep does this check internally.

thanks, will remove it.

> 
> Sorry for for the bits I should have posted in v1 of the patch. All of my
> comments are very minor, or cosmetic in nature, and, in my opinion, can just
> be applied by whoever pushes it.

thanks, no problem, just to make the code better. :)

> 
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel