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

[Derek Buitenhuis]

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