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.

> +            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;

> +                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/
> +                for (int y = starty; y < endy; y++) {
> +                    for (int x = startx; x < endx; x++) {
> +                        qoffsets[x + y*mbx] = qoffset;
> +                    }
> +                }

Cosmetic: Braces not necessary.

> +
> +                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.


>  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.

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.

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

Reply via email to