Re: [FFmpeg-devel] [PATCH V2] avcodec/libx265: add support for ROI-based encoding
> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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();
>
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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.
> +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();
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
[FFmpeg-devel] [PATCH V2] avcodec/libx265: add support for ROI-based encoding
Signed-off-by: Guo, Yejun
---
libavcodec/libx265.c | 67
1 file changed, 67 insertions(+)
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, );
+if (ret < 0)
+return ret;
}
ret = ctx->api->encoder_encode(ctx->encoder, , ,
pic ? : NULL, _out);
+
+if (x265pic.quantOffsets)
+av_freep();
+
if (ret < 0)
return AVERROR_EXTERNAL;
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
