| Message ID | 1548081614-13363-1-git-send-email-yejun.guo@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
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
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Derek Buitenhuis > Sent: Tuesday, January 22, 2019 10:31 PM > To: ffmpeg-devel@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 27c90b3..73a7e62 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -285,6 +285,65 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) return 0; } +static av_cold int libx265_encode_set_roi(libx265Context *ctx, const AVFrame *frame, x265_picture* pic) +{ + AVFrameSideData *sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST); + if (sd) { + if (ctx->params->rc.aqMode == X265_AQ_NONE) { + av_log(ctx, AV_LOG_WARNING, "Adaptive quantization must be enabled to use ROI encoding, skipping ROI.\n"); + } else { + // 8x8 block when qg-size is 8, 16*16 block otherwise + 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; + float* qoffsets; // will be freed after encode is called + qoffsets = av_mallocz_array(mbx * mby, sizeof(*qoffsets)); + if (!qoffsets) + return AVERROR(ENOMEM); + + nb_rois = sd->size / sizeof(AVRegionOfInterest); + roi = (AVRegionOfInterest*)sd->data; + for (int count = 0; count < nb_rois; count++) { + int starty = FFMIN(mby, roi->top / mb_size); + int endy = FFMIN(mby, (roi->bottom + mb_size - 1)/ mb_size); + int startx = FFMIN(mbx, roi->left / mb_size); + int endx = FFMIN(mbx, (roi->right + mb_size - 1)/ mb_size); + float qoffset; + + if (roi->qoffset.den == 0) { + av_free(qoffsets); + av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den should not be zero.\n"); + return AVERROR(EINVAL); + } + qoffset = roi->qoffset.num * 1.0f / roi->qoffset.den; + qoffset = av_clipf(qoffset, -1.0f, 1.0f); + + // qp range of x265 is from 0 to 51, just choose 25 as the scale value, + // so the range of final qoffset is [-25.0, 25.0]. + qoffset = qoffset * 25; + + for (int y = starty; y < endy; y++) { + for (int x = startx; x < endx; x++) { + qoffsets[x + y*mbx] = qoffset; + } + } + + 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); + } + roi = (AVRegionOfInterest*)((char*)roi + roi->self_size); + } + + pic->quantOffsets = qoffsets; + } + } + return 0; +} + static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *pic, int *got_packet) { @@ -314,10 +373,18 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt, pic->pict_type == AV_PICTURE_TYPE_P ? X265_TYPE_P : pic->pict_type == AV_PICTURE_TYPE_B ? X265_TYPE_B : X265_TYPE_AUTO; + + ret = libx265_encode_set_roi(ctx, pic, &x265pic); + if (ret < 0) + return ret; } ret = ctx->api->encoder_encode(ctx->encoder, &nal, &nnal, pic ? &x265pic : NULL, &x265pic_out); + + if (x265pic.quantOffsets) + av_freep(&x265pic.quantOffsets); + if (ret < 0) return AVERROR_EXTERNAL;
Signed-off-by: Guo, Yejun <yejun.guo@intel.com> --- libavcodec/libx265.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)