[FFmpeg-devel,V8,2/2] avcodec/libx264: add support for ROI-based encoding

Submitted by Guo, Yejun on Jan. 10, 2019, 8:54 a.m.

Details

Message ID 1547110470-1686-1-git-send-email-yejun.guo@intel.com
State New
Headers show

Commit Message

Guo, Yejun Jan. 10, 2019, 8:54 a.m.
This patch just enables the path from ffmpeg to libx264,
the more encoders can be added later.

Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
 libavcodec/libx264.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Comments

Carl Eugen Hoyos Jan. 10, 2019, 5:19 p.m.
2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> +                        roi = (AVRegionOfInterest*)((char*)roi +
> roi->self_size);

Isn't this roi++?
If yes, please change this.

I also wonder if the wording (elsewhere) of "returns EINVAL
if size is zero" is correct: Shouldn't it be "size must be >0"
or similar? A struct can hardly return an error, no?

Sorry for the late comment, Carl Eugen
Guo, Yejun Jan. 11, 2019, 12:37 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Friday, January 11, 2019 1:19 AM

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add support

> for ROI-based encoding

> 

> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> 

> > +                        roi = (AVRegionOfInterest*)((char*)roi +

> > roi->self_size);

> 

> Isn't this roi++?

> If yes, please change this.


no, it's not roi++, the reason is that struct AVRegionOfInterest might be extended,
so to keep ABI compatible, we add the 'self_size'.  It is discussed in V4 comments.

> 

> I also wonder if the wording (elsewhere) of "returns EINVAL if size is zero" is

> correct: Shouldn't it be "size must be >0"


self_size is uint32_t, it is > 0 if not zero.  


> or similar? A struct can hardly return an error, no?


it is caller's responsibility to set the value to be sizeof(AVRegionOfInterest).
There will be an error if the caller does not set it explicitly.

> 

> Sorry for the late comment, Carl Eugen

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Carl Eugen Hoyos Jan. 11, 2019, 12:53 a.m.
2019-01-11 1:37 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

>> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:
>>
>> > +                        roi = (AVRegionOfInterest*)((char*)roi +
>> > roi->self_size);
>>
>> Isn't this roi++?
>> If yes, please change this.
>
> no, it's not roi++, the reason is that struct AVRegionOfInterest might be
> extended,
> so to keep ABI compatible, we add the 'self_size'.  It is discussed in V4
> comments.

So after AVRegionOfInterest was extended, you as a user who had
compiled his application against old libavcodec will use new libavcodec
(with the extension) and it will be guaranteed that libx264 will still get
the correct information although only the part of the struct before the
extension was filled?
Does this guarantee really help?

>> I also wonder if the wording (elsewhere) of "returns EINVAL if
>> size is zero" is correct: Shouldn't it be "size must be >0"
>
> self_size is uint32_t, it is > 0 if not zero.

The comment I saw is in another file (probably another patch),
sorry if this is confusing (I also find it confusing).
I will hopefully not be affected by the documentation, I just wanted
to point out that the wording is misleading imo, because structs
do not return errors.

Carl Eugen
Guo, Yejun Jan. 11, 2019, 1:39 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Friday, January 11, 2019 8:54 AM

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add support

> for ROI-based encoding

> 

> 2019-01-11 1:37 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> 

> >> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> >>

> >> > +                        roi = (AVRegionOfInterest*)((char*)roi +

> >> > roi->self_size);

> >>

> >> Isn't this roi++?

> >> If yes, please change this.

> >

> > no, it's not roi++, the reason is that struct AVRegionOfInterest might

> > be extended, so to keep ABI compatible, we add the 'self_size'.  It is

> > discussed in V4 comments.

> 

> So after AVRegionOfInterest was extended, you as a user who had compiled

> his application against old libavcodec will use new libavcodec (with the

> extension) and it will be guaranteed that libx264 will still get the correct

> information although only the part of the struct before the extension was

> filled?

> Does this guarantee really help?


yes, I think it helps. otherwise, it becomes a possible ABI issue.

> 

> >> I also wonder if the wording (elsewhere) of "returns EINVAL if size

> >> is zero" is correct: Shouldn't it be "size must be >0"

> >

> > self_size is uint32_t, it is > 0 if not zero.

> 

> The comment I saw is in another file (probably another patch), sorry if this is

> confusing (I also find it confusing).

> I will hopefully not be affected by the documentation, I just wanted to point

> out that the wording is misleading imo, because structs do not return errors.


I see, it is in frame.h of patch 1/2.

imho, current wording is not misleading, the wording exactly matches the code, while the code is acceptable.
There is also an explicit note (very close these wording) that the correct value should be sizeof(AVRegionOfInterest).

Regarding "structs do not return errors": 

> or similar? A struct can hardly return an error, no?

it is caller's responsibility to set the value to be sizeof(AVRegionOfInterest).
There will be an error returned if the caller does not set it explicitly.

> 

> Carl Eugen

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Guo, Yejun Jan. 16, 2019, 12:41 a.m.
this patch set asks for pushing if no more concerns, thanks.

> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Guo, Yejun

> Sent: Friday, January 11, 2019 9:39 AM

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add support

> for ROI-based encoding

> 

> 

> 

> > -----Original Message-----

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Carl Eugen Hoyos

> > Sent: Friday, January 11, 2019 8:54 AM

> > To: FFmpeg development discussions and patches <ffmpeg-

> > devel@ffmpeg.org>

> > Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add

> > support for ROI-based encoding

> >

> > 2019-01-11 1:37 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> >

> > >> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>:

> > >>

> > >> > +                        roi = (AVRegionOfInterest*)((char*)roi +

> > >> > roi->self_size);

> > >>

> > >> Isn't this roi++?

> > >> If yes, please change this.

> > >

> > > no, it's not roi++, the reason is that struct AVRegionOfInterest

> > > might be extended, so to keep ABI compatible, we add the

> > > 'self_size'.  It is discussed in V4 comments.

> >

> > So after AVRegionOfInterest was extended, you as a user who had

> > compiled his application against old libavcodec will use new

> > libavcodec (with the

> > extension) and it will be guaranteed that libx264 will still get the

> > correct information although only the part of the struct before the

> > extension was filled?

> > Does this guarantee really help?

> 

> yes, I think it helps. otherwise, it becomes a possible ABI issue.

> 

> >

> > >> I also wonder if the wording (elsewhere) of "returns EINVAL if size

> > >> is zero" is correct: Shouldn't it be "size must be >0"

> > >

> > > self_size is uint32_t, it is > 0 if not zero.

> >

> > The comment I saw is in another file (probably another patch), sorry

> > if this is confusing (I also find it confusing).

> > I will hopefully not be affected by the documentation, I just wanted

> > to point out that the wording is misleading imo, because structs do not

> return errors.

> 

> I see, it is in frame.h of patch 1/2.

> 

> imho, current wording is not misleading, the wording exactly matches the

> code, while the code is acceptable.

> There is also an explicit note (very close these wording) that the correct value

> should be sizeof(AVRegionOfInterest).

> 

> Regarding "structs do not return errors":

> 

> > or similar? A struct can hardly return an error, no?

> it is caller's responsibility to set the value to be sizeof(AVRegionOfInterest).

> There will be an error returned if the caller does not set it explicitly.

> 

> >

> > Carl Eugen

> > _______________________________________________

> > 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
Derek Buitenhuis Jan. 16, 2019, 12:39 p.m.
On 16/01/2019 00:41, Guo, Yejun wrote:
> this patch set asks for pushing if no more concerns, thanks.

I support this.

If nobody raises any concerns in the next 24-48 hrs, I'll go ahead.

Thank you for sticking with it through the bikeshedding.

- Derek
Derek Buitenhuis Jan. 17, 2019, 9:55 p.m.
On 16/01/2019 12:39, Derek Buitenhuis wrote:
> On 16/01/2019 00:41, Guo, Yejun wrote:
>> this patch set asks for pushing if no more concerns, thanks.
> 
> I support this.
> 
> If nobody raises any concerns in the next 24-48 hrs, I'll go ahead.
> 
> Thank you for sticking with it through the bikeshedding.

Pushed.

- Derek

Patch hide | download patch | download mbox

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index a68d0a7..a3493f3 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -40,6 +40,10 @@ 
 #include <stdlib.h>
 #include <string.h>
 
+// from x264.h, for quant_offsets, Macroblocks are 16x16
+// blocks of pixels (with respect to the luma plane)
+#define MB_SIZE 16
+
 typedef struct X264Context {
     AVClass        *class;
     x264_param_t    params;
@@ -282,6 +286,7 @@  static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
     x264_picture_t pic_out = {0};
     int pict_type;
     int64_t *out_opaque;
+    AVFrameSideData *sd;
 
     x264_picture_init( &x4->pic );
     x4->pic.img.i_csp   = x4->params.i_csp;
@@ -345,6 +350,63 @@  static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
                 }
             }
         }
+
+        sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
+        if (sd) {
+            if (x4->params.rc.i_aq_mode == X264_AQ_NONE) {
+                av_log(ctx, AV_LOG_WARNING, "Adaptive quantization must be enabled to use ROI encoding, skipping ROI.\n");
+            } else {
+                if (frame->interlaced_frame == 0) {
+                    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;
+                    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);
+
+                        // 25 is a number that I think it is a possible proper scale value.
+                        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);
+                    }
+
+                    x4->pic.prop.quant_offsets = qoffsets;
+                    x4->pic.prop.quant_offsets_free = av_free;
+                } else {
+                    av_log(ctx, AV_LOG_WARNING, "interlaced_frame not supported for ROI encoding yet, skipping ROI.\n");
+                }
+            }
+        }
     }
 
     do {