[FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Guo, Yejun]

Signed-off-by: Guo, Yejun 
---
 libavcodec/libvpxenc.c | 132 +
 1 file changed, 132 insertions(+)

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index c823b8a..e3de547 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -1057,6 +1057,135 @@ static int queue_frames(AVCodecContext *avctx, AVPacket 
*pkt_out)
 return size;
 }
 
+static int vp8_encode_set_roi(AVCodecContext *avctx, const AVFrame *frame)
+{
+/* range of vpx_roi_map_t.delta_q[i] is [-63, 63] */
+#define MAX_DELTA_Q 63
+
+AVRegionOfInterest *roi = NULL;
+vpx_roi_map_t roi_map;
+AVFrameSideData *sd = av_frame_get_side_data(frame, 
AV_FRAME_DATA_REGIONS_OF_INTEREST);
+VPxContext *ctx = avctx->priv_data;
+vpx_active_map_t active_map = { 0, 0, 0 };
+int segment_id;
+int zero_delta_q = 0;
+
+/* record the mapping from delta_q to "segment id + 1".
+ * delta_q is shift with MAX_DELTA_Q, and so the range is [0, 
2*MAX_DELTA_Q].
+ * add 1 to segment id, so no mapping if the value of array element is 
zero.
+ */
+int segment_mapping[2 * MAX_DELTA_Q + 1] = {0};
+
+active_map.rows = (frame->height + 15) / 16;
+active_map.cols = (frame->width  + 15) / 16;
+
+if (!sd) {
+if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP, &active_map)) 
{
+log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec 
control.\n");
+return AVERROR_INVALIDDATA;
+}
+return 0;
+}
+
+active_map.active_map = av_malloc(active_map.rows * active_map.cols);
+if (!active_map.active_map) {
+av_log(avctx, AV_LOG_ERROR,
+   "active_map alloc (%d bytes) failed.\n",
+   active_map.rows * active_map.cols);
+return AVERROR(ENOMEM);
+}
+memset(active_map.active_map, 1, active_map.rows * active_map.cols);
+
+/* segment id 0 in roi_map is reserved for the areas not covered by 
AVRegionOfInterest.
+ * segment id 0 in roi_map is also for the areas with 
AVRegionOfInterest.qoffset near 0.
+ */
+segment_id = 0;
+segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
+segment_id++;
+memset(&roi_map, 0, sizeof(roi_map));
+roi_map.delta_q[segment_id] = zero_delta_q;
+
+roi_map.rows = active_map.rows;
+roi_map.cols = active_map.cols;
+roi_map.roi_map = av_mallocz(roi_map.rows * roi_map.cols);
+if (!roi_map.roi_map) {
+av_free(active_map.active_map);
+av_log(avctx, AV_LOG_ERROR,
+   "roi_map alloc (%d bytes) failed.\n",
+   roi_map.rows * roi_map.cols);
+return AVERROR(ENOMEM);
+}
+
+roi = (AVRegionOfInterest*)sd->data;
+while ((uint8_t*)roi < sd->data + sd->size) {
+int qoffset;
+int mapping_index;
+int mapping_value;
+int starty = FFMIN(roi_map.rows, roi->top / 16);
+int endy   = FFMIN(roi_map.rows, (roi->bottom + 15) / 16);
+int startx = FFMIN(roi_map.cols, roi->left / 16);
+int endx   = FFMIN(roi_map.cols, (roi->right + 15) / 16);
+
+if (roi->self_size == 0) {
+av_free(active_map.active_map);
+av_free(roi_map.roi_map);
+av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size must be 
set to sizeof(AVRegionOfInterest).\n");
+return AVERROR(EINVAL);
+}
+
+if (roi->qoffset.den == 0) {
+av_free(active_map.active_map);
+av_free(roi_map.roi_map);
+av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den must not 
be zero.\n");
+return AVERROR(EINVAL);
+}
+
+qoffset = (int)(roi->qoffset.num * 1.0f / roi->qoffset.den * 
MAX_DELTA_Q);
+qoffset = av_clip(qoffset, -MAX_DELTA_Q, MAX_DELTA_Q);
+
+mapping_index = qoffset + MAX_DELTA_Q;
+if (!segment_mapping[mapping_index]) {
+if (segment_id > 3) {
+av_log(ctx, AV_LOG_WARNING,
+   "ROI only support 4 segments (and segment 0 is reserved 
for non-ROIs), skipping this one.\n");
+roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
+continue;
+}
+
+segment_mapping[mapping_index] = segment_id + 1;
+roi_map.delta_q[segment_id] = qoffset;
+segment_id++;
+}
+
+mapping_value = segment_mapping[mapping_index];
+
+for (int y = starty; y < endy; y++)
+for (int x = startx; x < endx; x++)
+roi_map.roi_map[x + y * roi_map.cols] = mapping_value - 1;
+
+roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
+}
+
+if (vpx_codec_control(&ctx->encoder, VP8E_SET_ROI_MAP, &roi_map)) {
+av_free(active_map.active_map);
+av_free(roi_map.roi_map);
+log_encoder_error(avctx, "Failed to set VP8E_SET_ROI_MAP codec 
control.\n");
+return AVERROR_INVALIDDATA;
+}
+
+if (vpx_co

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Guo, Yejun]

> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Guo, Yejun
> Sent: Thursday, February 28, 2019 12:13 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for
> ROI-based encoding
> 
> Signed-off-by: Guo, Yejun 
> ---
>  libavcodec/libvpxenc.c | 132
> +
>  1 file changed, 132 insertions(+)
> 
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index c823b8a..e3de547 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -1057,6 +1057,135 @@ static int queue_frames(AVCodecContext *avctx,
> AVPacket *pkt_out)
>  return size;
>  }
> 
> +static int vp8_encode_set_roi(AVCodecContext *avctx, const AVFrame
> *frame)
> +{
> +/* range of vpx_roi_map_t.delta_q[i] is [-63, 63] */
> +#define MAX_DELTA_Q 63
> +
> +AVRegionOfInterest *roi = NULL;
> +vpx_roi_map_t roi_map;
> +AVFrameSideData *sd = av_frame_get_side_data(frame,
> AV_FRAME_DATA_REGIONS_OF_INTEREST);
> +VPxContext *ctx = avctx->priv_data;
> +vpx_active_map_t active_map = { 0, 0, 0 };
> +int segment_id;
> +int zero_delta_q = 0;
> +
> +/* record the mapping from delta_q to "segment id + 1".
> + * delta_q is shift with MAX_DELTA_Q, and so the range is [0,
> 2*MAX_DELTA_Q].
> + * add 1 to segment id, so no mapping if the value of array element is 
> zero.
> + */
> +int segment_mapping[2 * MAX_DELTA_Q + 1] = {0};
> +
> +active_map.rows = (frame->height + 15) / 16;
> +active_map.cols = (frame->width  + 15) / 16;
> +
> +if (!sd) {
> +if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP,
> &active_map)) {
> +log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec
> control.\n");
> +return AVERROR_INVALIDDATA;
> +}
> +return 0;
> +}
> +
> +active_map.active_map = av_malloc(active_map.rows * active_map.cols);
> +if (!active_map.active_map) {
> +av_log(avctx, AV_LOG_ERROR,
> +   "active_map alloc (%d bytes) failed.\n",
> +   active_map.rows * active_map.cols);
> +return AVERROR(ENOMEM);
> +}
> +memset(active_map.active_map, 1, active_map.rows * active_map.cols);
> +
> +/* segment id 0 in roi_map is reserved for the areas not covered by
> AVRegionOfInterest.
> + * segment id 0 in roi_map is also for the areas with
> AVRegionOfInterest.qoffset near 0.
> + */
> +segment_id = 0;
> +segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
> +segment_id++;
> +memset(&roi_map, 0, sizeof(roi_map));
> +roi_map.delta_q[segment_id] = zero_delta_q;
> +
> +roi_map.rows = active_map.rows;
> +roi_map.cols = active_map.cols;
> +roi_map.roi_map = av_mallocz(roi_map.rows * roi_map.cols);
> +if (!roi_map.roi_map) {
> +av_free(active_map.active_map);
> +av_log(avctx, AV_LOG_ERROR,
> +   "roi_map alloc (%d bytes) failed.\n",
> +   roi_map.rows * roi_map.cols);
> +return AVERROR(ENOMEM);
> +}
> +
> +roi = (AVRegionOfInterest*)sd->data;
> +while ((uint8_t*)roi < sd->data + sd->size) {
> +int qoffset;
> +int mapping_index;
> +int mapping_value;
> +int starty = FFMIN(roi_map.rows, roi->top / 16);
> +int endy   = FFMIN(roi_map.rows, (roi->bottom + 15) / 16);
> +int startx = FFMIN(roi_map.cols, roi->left / 16);
> +int endx   = FFMIN(roi_map.cols, (roi->right + 15) / 16);
> +
> +if (roi->self_size == 0) {
> +av_free(active_map.active_map);
> +av_free(roi_map.roi_map);
> +av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size must be
> set to sizeof(AVRegionOfInterest).\n");
> +return AVERROR(EINVAL);
> +}
> +
> +if (roi->qoffset.den == 0) {
> +av_free(active_map.active_map);
> +av_free(roi_map.roi_map);
> +av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den must
> not be zero.\n");
> +return AVERROR(EINVAL);
> +}
> +
> +qoffset = (int)(roi->qoffset.num * 1.0f / roi->qoffset.den *
> MAX_DELTA_Q);
> +qoffset = av_clip(qoffset, -MAX_DELTA_Q, MAX_DELTA_Q);
> +
> +mapping_index = qoffset + MAX_DELTA_Q;
> +if (!segment_mapping[mapping_index]) {
> +if (segment_id > 3) {
> +av_log(ctx, AV_LOG_WARNING,
> +   "ROI only support 4 segments (and segment 0 is 
> reserved for
> non-ROIs), skipping this one.\n");
> +roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
> +continue;
> +}
> +
> +segment_mapping[mapping_index] = segment_id + 1;
> +roi_map.delta_q[segment_id] = qoffset;
> +segment_id++;
> +}
> +
> +mapping_value = segment_mapping[mapping_index];
> +
> +  

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Gyan]

On 27-02-2019 01:57 PM, Guo, Yejun wrote:



-Original Message-
From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
Of Guo, Yejun
Sent: Thursday, February 28, 2019 12:13 AM
To: ffmpeg-devel@ffmpeg.org
Subject: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for
ROI-based encoding

Signed-off-by: Guo, Yejun 
---
  libavcodec/libvpxenc.c | 132
+
  1 file changed, 132 insertions(+)

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index c823b8a..e3de547 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -1057,6 +1057,135 @@ static int queue_frames(AVCodecContext *avctx,
AVPacket *pkt_out)
  return size;
  }

+static int vp8_encode_set_roi(AVCodecContext *avctx, const AVFrame
*frame)
+{
+/* range of vpx_roi_map_t.delta_q[i] is [-63, 63] */
+#define MAX_DELTA_Q 63
+
+AVRegionOfInterest *roi = NULL;
+vpx_roi_map_t roi_map;
+AVFrameSideData *sd = av_frame_get_side_data(frame,
AV_FRAME_DATA_REGIONS_OF_INTEREST);
+VPxContext *ctx = avctx->priv_data;
+vpx_active_map_t active_map = { 0, 0, 0 };
+int segment_id;
+int zero_delta_q = 0;
+
+/* record the mapping from delta_q to "segment id + 1".
+ * delta_q is shift with MAX_DELTA_Q, and so the range is [0,
2*MAX_DELTA_Q].
+ * add 1 to segment id, so no mapping if the value of array element is 
zero.
+ */
+int segment_mapping[2 * MAX_DELTA_Q + 1] = {0};
+
+active_map.rows = (frame->height + 15) / 16;
+active_map.cols = (frame->width  + 15) / 16;
+
+if (!sd) {
+if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP,
&active_map)) {
+log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec
control.\n");
+return AVERROR_INVALIDDATA;
+}
+return 0;
+}
+
+active_map.active_map = av_malloc(active_map.rows * active_map.cols);
+if (!active_map.active_map) {
+av_log(avctx, AV_LOG_ERROR,
+   "active_map alloc (%d bytes) failed.\n",
+   active_map.rows * active_map.cols);
+return AVERROR(ENOMEM);
+}
+memset(active_map.active_map, 1, active_map.rows * active_map.cols);
+
+/* segment id 0 in roi_map is reserved for the areas not covered by
AVRegionOfInterest.
+ * segment id 0 in roi_map is also for the areas with
AVRegionOfInterest.qoffset near 0.
+ */
+segment_id = 0;
+segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
+segment_id++;
+memset(&roi_map, 0, sizeof(roi_map));
+roi_map.delta_q[segment_id] = zero_delta_q;
+
+roi_map.rows = active_map.rows;
+roi_map.cols = active_map.cols;
+roi_map.roi_map = av_mallocz(roi_map.rows * roi_map.cols);
+if (!roi_map.roi_map) {
+av_free(active_map.active_map);
+av_log(avctx, AV_LOG_ERROR,
+   "roi_map alloc (%d bytes) failed.\n",
+   roi_map.rows * roi_map.cols);
+return AVERROR(ENOMEM);
+}
+
+roi = (AVRegionOfInterest*)sd->data;
+while ((uint8_t*)roi < sd->data + sd->size) {
+int qoffset;
+int mapping_index;
+int mapping_value;
+int starty = FFMIN(roi_map.rows, roi->top / 16);
+int endy   = FFMIN(roi_map.rows, (roi->bottom + 15) / 16);
+int startx = FFMIN(roi_map.cols, roi->left / 16);
+int endx   = FFMIN(roi_map.cols, (roi->right + 15) / 16);
+
+if (roi->self_size == 0) {
+av_free(active_map.active_map);
+av_free(roi_map.roi_map);
+av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size must be
set to sizeof(AVRegionOfInterest).\n");
+return AVERROR(EINVAL);
+}
+
+if (roi->qoffset.den == 0) {
+av_free(active_map.active_map);
+av_free(roi_map.roi_map);
+av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den must
not be zero.\n");
+return AVERROR(EINVAL);
+}
+
+qoffset = (int)(roi->qoffset.num * 1.0f / roi->qoffset.den *
MAX_DELTA_Q);
+qoffset = av_clip(qoffset, -MAX_DELTA_Q, MAX_DELTA_Q);
+
+mapping_index = qoffset + MAX_DELTA_Q;
+if (!segment_mapping[mapping_index]) {
+if (segment_id > 3) {
+av_log(ctx, AV_LOG_WARNING,
+   "ROI only support 4 segments (and segment 0 is reserved 
for
non-ROIs), skipping this one.\n");
+roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
+continue;
+}
+
+segment_mapping[mapping_index] = segment_id + 1;
+roi_map.delta_q[segment_id] = qoffset;
+segment_id++;
+}
+
+mapping_value = segment_mapping[mapping_index];
+
+for (int y = starty; y < endy; y++)
+for (int x = startx; x < endx; x++)
+roi_map.roi_map[x + y * roi_map.cols] = mapping_value - 1;
+
+roi = (AVRegionOfInterest*)((char*)roi + roi->self

[FFmpeg-devel] ffmpeg dylibs for OSX

[Patrick Cusack]

Does anyone have any experience building ffmpeg dylibs for Xcode? I want to 
include ffmpeg shared libraries in my macOS binary bundle for easy 
distribution. 

Patrick
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mov: fix hang while seek on a kind of fragmented mp4.

[Michael Niedermayer]

On Mon, Feb 25, 2019 at 07:07:27PM +0800, C.H.Liu wrote:
>  Would you mind share your input file? I still can’t reproduce the OOM.

i belive i cannot share this sample, but the patch really needs to be
fully reviewed not just one bug that was found by chance fixed otherwise
we might be missing other bugs ...


> 
> 
> I have tried several types of mp4.
> 
> I also tried to create a file like yours. According to the snapshot, seems
> it has no ‘sidx’ box and enable ‘use_mfra_for’.
> 
> Massif didn’t report update_frag_index() function. I didn’t see an extra
> heap as big as here, either.

looking at the sample that triggers the OOM the do/while loop you add 
to update_frag_index() is entered and never exits while doing allocations
in it.

also the patch as is is not ok. You for example have a list of unrelated
changes in the commit message. Each should be in its own patch
also changes that are functional should be splited from non functional
changes. For example here:

 // If moof_offset already exists in frag_index, return index to it
-index = search_frag_moof_offset(&c->frag_index, offset);
-if (index < c->frag_index.nb_items &&
-c->frag_index.item[index].moof_offset == offset)
+index = search_frag_moof_offset(frag_index, offset);
+if (index < frag_index->nb_items &&
+frag_index->item[index].moof_offset == offset) {
+frag_index->current = index;
 return index;
+}

you change frag_index to a local variable this is non functional and
should not be in a patch that does a functional change ideally.
Now sure if a functional change is trivial and clear such things are
sometimes done together but this patch is not clear, at least its not
clear to me ...

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Derek Buitenhuis]

On 27/02/2019 10:22, Gyan wrote:
> Looks like, at present, the only way to effect ROI is via side data, and 
> no filter or other in-tree mechanism at present can convey or generate it.

Life exists outside of ffmpeg.c, and it's an extremely useful API to have.

> Are there any near-term plans to add some? If not, may I suggest that 
> you add a private option for supporting encoders, for users to input ROIs?

Please don't suggest removing an API that was *just added*, after many months
of bikeshedding. Please.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Gyan]

On 27-02-2019 07:07 PM, Derek Buitenhuis wrote:

On 27/02/2019 10:22, Gyan wrote:

Looks like, at present, the only way to effect ROI is via side data, and
no filter or other in-tree mechanism at present can convey or generate it.

Life exists outside of ffmpeg.c, and it's an extremely useful API to have.


Are there any near-term plans to add some? If not, may I suggest that
you add a private option for supporting encoders, for users to input ROIs?

Please don't suggest removing an API that was *just added*, after many months
of bikeshedding. Please.


Huh. I haven't suggested removing anything.

I suggested *adding* a way for this feature to be useful for ffmpeg 
users in the near-term. Who knows how long will it take for a decent 
per-frame ROI filter, like the facedetect example mentioned in the 
initial discussion.


Gyan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix sdp size check on fmtp integer parameters

[Michael Niedermayer]

On Mon, Feb 25, 2019 at 02:54:50PM +0100, Olivier Maignial wrote:
> ---
>  libavformat/rtpdec_mpeg4.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..f632ebf 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,15 @@ static int parse_fmtp(AVFormatContext *s,
>  for (i = 0; attr_names[i].str; ++i) {
>  if (!av_strcasecmp(attr, attr_names[i].str)) {
>  if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> -int val = atoi(value);
> -if (val > 32) {
> +long int val = strtol(value, NULL, 10);
> +if (errno == ERANGE || val > INT_MAX || val < INT_MIN) {

i belive strtol can fail with other errno values


>  av_log(s, AV_LOG_ERROR,
> -   "The %s field size is invalid (%d)\n",
> +   "The %s field size is invalid (%ld)\n",
> attr, val);
>  return AVERROR_INVALIDDATA;
>  }

>  *(int *)((char *)data+
> -attr_names[i].offset) = val;
> +attr_names[i].offset) = (int) val;

this is not needed, though it does no harm if the intend is to make it
clear that the type is converted intentionally here

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix sdp size check on fmtp integer parameters

[Nicolas George]

Michael Niedermayer (12019-02-27):
> >  if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> > -int val = atoi(value);
> > -if (val > 32) {
> > +long int val = strtol(value, NULL, 10);
> > +if (errno == ERANGE || val > INT_MAX || val < INT_MIN) 
> > {
> 
> i belive strtol can fail with other errno values

And errno is not reset in case of successful completion. It needs to be
cleared beforehand.

To check if a conversion error happened, the middle argument is the
proper tool.

Regards,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Parallelize vf_lut

[Michael Niedermayer]

On Mon, Feb 25, 2019 at 03:25:30PM -0500, Britt Cyr wrote:
> ---
>  libavfilter/vf_lut.c | 106 ---
>  1 file changed, 70 insertions(+), 36 deletions(-)
> 
> diff --git a/libavfilter/vf_lut.c b/libavfilter/vf_lut.c
> index c815ddc194..14386938be 100644
> --- a/libavfilter/vf_lut.c
> +++ b/libavfilter/vf_lut.c
> @@ -72,6 +72,12 @@ typedef struct LutContext {
>  int negate_alpha; /* only used by negate */
>  } LutContext;
>  
> +typedef struct ThreadData {
> +  AVFrame *in;
> +  AVFrame *out;
> +  AVFilterLink *link;
> +} ThreadData;

indention depth is inconsistant


[...]
> @@ -366,11 +359,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame 
> *in)
>  const int in_linesize  =  in->linesize[0] / 2;
>  const int out_linesize = out->linesize[0] / 2;
>  const int step = s->step;
> +const int row_min = jobnr / nb_jobs * h;
> +const int row_max = (jobnr + 1) / nb_jobs * h;
>  
>  inrow0  = (uint16_t*) in ->data[0];
>  outrow0 = (uint16_t*) out->data[0];
>  
> -for (i = 0; i < h; i ++) {
> +for (i = row_min; i < row_max; i ++) {
>  inrow  = inrow0;
>  outrow = outrow0;
>  for (j = 0; j < w; j++) {
> @@ -403,11 +398,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame 
> *in)
>  const int in_linesize  =  in->linesize[0];
>  const int out_linesize = out->linesize[0];
>  const int step = s->step;
> +const int row_min = jobnr / nb_jobs * h;
> +const int row_max = (jobnr + 1) / nb_jobs * h;
>  
>  inrow0  = in ->data[0];
>  outrow0 = out->data[0];
>  
> -for (i = 0; i < h; i ++) {
> +for (i = row_min; i < row_max; i ++) {
>  inrow  = inrow0;
>  outrow = outrow0;
>  for (j = 0; j < w; j++) {
> @@ -435,11 +432,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame 
> *in)
>  const uint16_t *tab = s->lut[plane];
>  const int in_linesize  =  in->linesize[plane] / 2;
>  const int out_linesize = out->linesize[plane] / 2;
> +const int row_min = jobnr / nb_jobs * h;
> +const int row_max = (jobnr + 1) / nb_jobs * h;
>  
>  inrow  = (uint16_t *)in ->data[plane];
>  outrow = (uint16_t *)out->data[plane];
>  
> -for (i = 0; i < h; i++) {
> +for (i = row_min; i < row_max; i++) {
>  for (j = 0; j < w; j++) {
>  #if HAVE_BIGENDIAN
>  outrow[j] = av_bswap16(tab[av_bswap16(inrow[j])]);
> @@ -463,11 +462,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame 
> *in)
>  const uint16_t *tab = s->lut[plane];
>  const int in_linesize  =  in->linesize[plane];
>  const int out_linesize = out->linesize[plane];
> +const int row_min = jobnr / nb_jobs * h;
> +const int row_max = (jobnr + 1) / nb_jobs * h;
>  
>  inrow  = in ->data[plane];
>  outrow = out->data[plane];
>  
> -for (i = 0; i < h; i++) {
> +for (i = row_min; i < row_max; i++) {
>  for (j = 0; j < w; j++)
>  outrow[j] = tab[inrow[j]];
>  inrow  += in_linesize;

unreaĺated to your patch, i just spoted this as it makes it obvious
replicating this code 4 times is a bit ugly


> @@ -476,9 +477,42 @@ static int filter_frame(AVFilterLink *inlink, AVFrame 
> *in)
>  }
>  }
>  
> -if (!direct)
> +return 0;
> +}
> +
> +static AVFrame *apply_lut(AVFilterLink *inlink, AVFrame *in) {
> +AVFilterContext *ctx = inlink->dst;
> +AVFilterLink *outlink = ctx->outputs[0];
> +AVFrame *out;
> +ThreadData td;
> +
> +if (av_frame_is_writable(in)) {
> +out = in;
> +} else {
> +out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> +if (!out) {
> +av_frame_free(&in);
> +return NULL;
> +}
> +av_frame_copy_props(out, in);
> +}
> +td.in  = in;
> +td.out = out;
> +td.link = inlink;

> +ctx->internal->execute(ctx, lookup_slice, &td, NULL, FFMIN(outlink->h, 
> 1));

how many tasks does this run in parallel and how much faster is it ?

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] avcodec/proresenc_aw : improve speed by replacing PutBitContext for codeword encoding

[Martin Vignali]

>
> Shouldn’t there be a 64Bit PutBitContext instead so other encoders can
> also profit?
>
> Carl Eugen
>

I only use here a small part of putbitcontext, and in an "unsafe" mode (the
buffer of the prores aw encoder is big enough to not check it, and write 64
bits during the flush even if less can be use (to keep code more simple))

Maybe can be used later as a start to write a more general PutBitContext
with 64 bits bit_buf, for use in various encoders.


Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Derek Buitenhuis]

On 27/02/2019 13:48, Gyan wrote:
> Huh. I haven't suggested removing anything.
> 
> I suggested *adding* a way for this feature to be useful for ffmpeg 
> users in the near-term. Who knows how long will it take for a decent 
> per-frame ROI filter, like the facedetect example mentioned in the 
> initial discussion.

Apologies, I misread.

As for an AVOption: That could get ugly, fast. What do you pass it?
A string of ROI and offsets? That would balloon fast.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate/proresenc_aw : Add fate test for interlace and 444 encoding

[Martin Vignali]

> works here
>
>
 Pushed, thanks.

Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] GSoC 2019

[Martin Vignali]

Hello,

I don't have knowledge to mentor a project,

But two idea :

- Swscale improvement :
YAP8, YAP16 : useful to switch some decoder/encoder to planar
Float/half float pixel format support

- Open Exr Decoder :
Add DWA decoder
support 4:2:0 mode

Martin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Derek Buitenhuis]

On 27/02/2019 16:12, Guo, Yejun wrote:
> Signed-off-by: Guo, Yejun 
> ---
>  libavcodec/libvpxenc.c | 132 
> +
>  1 file changed, 132 insertions(+)

Do these APIs exist for all supported libvpx versions?

> +if (!sd) {
> +if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP, 
> &active_map)) {
> +log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec 
> control.\n");
> +return AVERROR_INVALIDDATA;
> +}

Why do this stuff at all if no ROIs have been set?

> +memset(active_map.active_map, 1, active_map.rows * active_map.cols);

Nit: Maybe a comment about what '1' is here.

> +
> +/* segment id 0 in roi_map is reserved for the areas not covered by 
> AVRegionOfInterest.
> + * segment id 0 in roi_map is also for the areas with 
> AVRegionOfInterest.qoffset near 0.
> + */
> +segment_id = 0;
> +segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
> +segment_id++;

This series of ops seems weirdly redundant separately?

> +memset(&roi_map, 0, sizeof(roi_map));

Can't zero-init?

> +av_free(active_map.active_map);
> +av_log(avctx, AV_LOG_ERROR,
> +   "roi_map alloc (%d bytes) failed.\n",
> +   roi_map.rows * roi_map.cols);

Here, and elsewhere: We don't write how many bytes failed, generally.

> +if (vpx_codec_control(&ctx->encoder, VP8E_SET_ROI_MAP, &roi_map)) {
> +av_free(active_map.active_map);
> +av_free(roi_map.roi_map);
> +log_encoder_error(avctx, "Failed to set VP8E_SET_ROI_MAP codec 
> control.\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP, &active_map)) {
> +av_free(active_map.active_map);
> +av_free(roi_map.roi_map);
> +log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP codec 
> control.\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +av_free(active_map.active_map);
> +av_free(roi_map.roi_map);

With the amount of failure areas in this function, is it reasonable to roll it
into one goto fail or something?

> +if (avctx->codec_id == AV_CODEC_ID_VP8)
> +vp8_encode_set_roi(avctx, frame);

The API only exists for VP8, or is this due to different quant scales or 
something?

As an aside, I must say, libvpx's ROI API is real ugly.

Cheers,
- Derek

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Gyan]

On 27-02-2019 10:32 PM, Derek Buitenhuis wrote:

On 27/02/2019 13:48, Gyan wrote:

Huh. I haven't suggested removing anything.

I suggested *adding* a way for this feature to be useful for ffmpeg
users in the near-term. Who knows how long will it take for a decent
per-frame ROI filter, like the facedetect example mentioned in the
initial discussion.

Apologies, I misread.

As for an AVOption: That could get ugly, fast. What do you pass it?
A string of ROI and offsets? That would balloon fast.
Yes,  a string. 4 integers and qp offset so either two more, in rational 
form, or a float.


There's already multiple interfaces accepting long multi-entry 
multi-valued arguments, whether custom ones in filters like curves or 
pan, or x264opts, or with the assistance of an existing key-value parser 
like x264-params. Unless the plan is to allow only machine-generated 
ROIs, some form of manual user specification will be needed, and that 
option will have to be prepared to accept and parse such a string.


Gyan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Derek Buitenhuis]

On 27/02/2019 17:31, Gyan wrote:
> Yes, a string. 4 integers and qp offset so either two more, in rational 
> form, or a float.

You're assuming a single ROI, though, which is only one case. Plenty of things
would need >1.

> There's already multiple interfaces accepting long multi-entry 
> multi-valued arguments, whether custom ones in filters like curves or 
> pan, or x264opts, or with the assistance of an existing key-value parser 
> like x264-params. Unless the plan is to allow only machine-generated 
> ROIs, some form of manual user specification will be needed, and that 
> option will have to be prepared to accept and parse such a string.

Feel free to send a patch then I guess... it doesn't sound very appealing
to use them this way, and certaily not to manually add an AVOption for
every codec.

I am not convinced using it manually via CLI options is at all appealing.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Derek Buitenhuis]

On 27/02/2019 17:57, Derek Buitenhuis wrote:
> You're assuming a single ROI, though, which is only one case. Plenty of things
> would need >1.

Also note that sets of ROIs are per-frame.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ffmpeg dylibs for OSX

[Helmut K. C. Tessarek]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On 2019-02-27 08:12, Patrick Cusack wrote:
> Does anyone have any experience building ffmpeg dylibs for Xcode? I
> want to include ffmpeg shared libraries in my macOS binary bundle
> for easy distribution.

I think you have to be a bit more specific. What dylibs? Are you
talking about the ffmpeg internal ones?

libavcodec
libavutil
libpostproc
libswresample
libswscale
libavformat
libavdevice
libavfilter

What about 3rd party libs?

Also, it might not be that easy to use dynamic libs in a bundle, since
you might have to change the rpath after the fact.

But vlc and/or Handbrake use ffmpeg libs in their products. I suggest
you ask them how they do it. IMO, it's rather a PAIN.

Cheers,
 K. C.

- -- 
regards Helmut K. C. Tessarek  KeyID 0x172380A011EF4944
Key fingerprint = 8A55 70C1 BD85 D34E ADBC 386C 1723 80A0 11EF 4944

/*
   Thou shalt not follow the NULL pointer for chaos and madness
   await thee at its end.
*/
-BEGIN PGP SIGNATURE-

iQIzBAEBCgAdFiEE191csiqpm8f5Ln9WvgmFNJ1E3QAFAlx22f4ACgkQvgmFNJ1E
3QA9wBAAl7SbD0HcNuOXP7QZkLHxeP+KVb0ABRUkPAkudfa7EHr4jC2rNlK3NziB
XIvrtXs0FKPgqbI9EttfvMJvSBJTywzELGlORpUTy2mxAPPCMImiwZ9D8Ei31HdQ
ri3SWxfooKlSJe9OHK2vnx3bb9opRasDVn+EL+UJCiHWG8gMGPkQsSPH/NEGfszr
ubOkGo7D5fCLKBpNI8+zI+SoW8zqY2c2qtBlRFiTmQ8/f/7dDigzP3zByAZj8me6
WfIB3ZqCoZrHx+eV54QfWL3nCdjIuEewi7y311/W7+ctR6wB2YA8eaCDMu62Rfyf
0aGj72Bp63n9Uoyg9Nogahlq/KBRZrjbvVNV4R6EJH/ScSLN1rlPAViEB7eVPlwQ
uJ78g4+NIfyBK5cR5rPjLK0d+U1qWn0JiTWTlQWU6Qf7gT8Z0g4jeudHVIinDnT+
5VddAnsYaiYxI5k3iqBmv46u2boVWgmKbRi4mFmIcaC6Z4HmQ4cHJdQxcXPiiWHD
Rj8WMHLJ4zEpFcm7QgPMNRLcI6EKyeS0rRIAsQC3CJTCu0z4aORbnzXEo9i9SBbU
T8lWK+eleorg7IeUkIW7R2UT7MzOfYmJ61m49fgGDgcPajoL8abTkb6upjXDp8rw
Pvn+IGqDkKT/wrCsPICKq50FHmoRI4R16XU8TWKT9tZ/BiIfTqU=
=hoxw
-END PGP SIGNATURE-
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Blackfin optimizations

[Michael Noel]

Hello,

Can you pls. send me more details about the Blackfin optimizisations for
the ffmpeg?

Thanks
Miki
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Blackfin optimizations

[Lou Logan]

On Wed, Feb 27, 2019, at 10:03 AM, Michael Noel wrote:
> Hello,
> 
> Can you pls. send me more details about the Blackfin optimizisations for
> the ffmpeg?
> 
> Thanks
> Miki

Blackfin optimization was removed from the ffmpeg years ago:


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 0/5] Clean up CUDA SDK usage and remove non-free requirement

[Timo Rothenpieler]

On 21.02.2019 04:57, Philip Langdale wrote:

I've been thinking about this for a while, but I only recently made the
realisation that compiling cuda kernels to the ptx format does not
introduce any non-free dependencies - the ptx files are an intermediate
assembly code format that is actually compiled to binary form at
runtime. With that understood, we just need to switch the remaining
users of the CUDA SDK to ffnvcodec and we will remove the non-free
entanglements from cuda.

Philip Langdale (5):
   configure: Add an explicit check and option for nvcc
   avfilter/vf_yadif_cuda: Switch to using ffnvcodec
   avfilter/vf_scale_cuda: Switch to using ffnvcodec
   avfilter/vf_thumbnail_cuda: Switch to using ffnvcodec
   configure: Remove cuda_sdk dependency option


Series applied, but kept cuda_nvcc non-free.



smime.p7s
Description: S/MIME Cryptographic Signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 5/5] lavfi: addroi filter

[Mark Thompson]

This can be used to add region of interest side data to video frames.
---
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_addroi.c  | 237 +++
 3 files changed, 239 insertions(+)
 create mode 100644 libavfilter/vf_addroi.c

diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index fef6ec5c55..31ae738a50 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -149,6 +149,7 @@ OBJS-$(CONFIG_SINE_FILTER)   += asrc_sine.o
 OBJS-$(CONFIG_ANULLSINK_FILTER)  += asink_anullsink.o
 
 # video filters
+OBJS-$(CONFIG_ADDROI_FILTER) += vf_addroi.o
 OBJS-$(CONFIG_ALPHAEXTRACT_FILTER)   += vf_extractplanes.o
 OBJS-$(CONFIG_ALPHAMERGE_FILTER) += vf_alphamerge.o
 OBJS-$(CONFIG_AMPLIFY_FILTER)+= vf_amplify.o
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index c51ae0f3c7..52413c8f0d 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -140,6 +140,7 @@ extern AVFilter ff_asrc_sine;
 
 extern AVFilter ff_asink_anullsink;
 
+extern AVFilter ff_vf_addroi;
 extern AVFilter ff_vf_alphaextract;
 extern AVFilter ff_vf_alphamerge;
 extern AVFilter ff_vf_amplify;
diff --git a/libavfilter/vf_addroi.c b/libavfilter/vf_addroi.c
new file mode 100644
index 00..c8d323e660
--- /dev/null
+++ b/libavfilter/vf_addroi.c
@@ -0,0 +1,237 @@
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/eval.h"
+#include "libavutil/opt.h"
+#include "avfilter.h"
+#include "internal.h"
+
+enum {
+TOP,
+BOTTOM,
+LEFT,
+RIGHT,
+};
+
+static const char *const addroi_var_names[] = {
+"w",
+"h",
+};
+
+enum {
+VAR_W,
+VAR_H,
+NB_VARS,
+};
+
+typedef struct AddROIContext {
+const AVClass *class;
+
+char *region_str[4];
+
+AVExpr *region_expr[4];
+double vars[NB_VARS];
+
+int region[4];
+AVRational qoffset;
+} AddROIContext;
+
+static int addroi_config_input(AVFilterLink *inlink)
+{
+AVFilterContext *avctx = inlink->dst;
+AddROIContext *ctx = avctx->priv;
+int i;
+double val;
+
+ctx->vars[VAR_W] = inlink->w;
+ctx->vars[VAR_H] = inlink->h;
+
+for (i = 0; i < 4; i++) {
+val = av_expr_eval(ctx->region_expr[i], ctx->vars, NULL);
+ctx->region[i] = av_clip(rint(val), 0,
+ i < 2 ? inlink->h : inlink->w);
+}
+
+return 0;
+}
+
+static int addroi_filter_frame(AVFilterLink *inlink, AVFrame *frame)
+{
+AVFilterContext *avctx = inlink->dst;
+AVFilterLink  *outlink = avctx->outputs[0];
+AddROIContext *ctx = avctx->priv;
+AVRegionOfInterest *roi;
+AVFrameSideData *sd;
+int err;
+
+sd = av_frame_get_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
+if (sd) {
+const AVRegionOfInterest *old_roi;
+AVBufferRef *roi_ref;
+int nb_roi, i;
+
+old_roi = (const AVRegionOfInterest*)sd->data;
+nb_roi = sd->size / old_roi->self_size + 1;
+
+roi_ref = av_buffer_alloc(sizeof(*roi) * nb_roi);
+if (!roi_ref) {
+err = AVERROR(ENOMEM);
+goto fail;
+}
+roi = (AVRegionOfInterest*)roi_ref->data;
+
+for (i = 0; i < nb_roi - 1; i++) {
+old_roi = (const AVRegionOfInterest*)
+(sd->data + old_roi->self_size * i);
+
+roi[i] = (AVRegionOfInterest) {
+.self_size = sizeof(*roi),
+.top   = old_roi->top,
+.bottom= old_roi->bottom,
+.left  = old_roi->left,
+.right = old_roi->right,
+.qoffset   = old_roi->qoffset,
+};
+}
+
+roi[nb_roi - 1] = (AVRegionOfInterest) {
+.self_size = sizeof(*roi),
+.top   = ctx->region[TOP],
+.bottom= ctx->region[BOTTOM],
+.left  = ctx->region[LEFT],
+.right = ctx->region[RIGHT],
+.qoffset   = ctx->qoffset,
+};
+
+av_frame_remove_side_data(frame, AV_FRAME_DATA_REGIONS_OF_INTEREST);
+
+sd = av_frame_new_side_data_from_buf(frame,
+ AV_

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for ROI-based encoding

[Mark Thompson]

On 27/02/2019 10:22, Gyan wrote:
> On 27-02-2019 01:57 PM, Guo, Yejun wrote:
>>
>>> -Original Message-
>>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
>>> Of Guo, Yejun
>>> Sent: Thursday, February 28, 2019 12:13 AM
>>> To: ffmpeg-devel@ffmpeg.org
>>> Subject: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: add VP8 support for
>>> ROI-based encoding
>>>
>>> Signed-off-by: Guo, Yejun 
>>> ---
>>>   libavcodec/libvpxenc.c | 132
>>> +
>>>   1 file changed, 132 insertions(+)
>>>
>>> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
>>> index c823b8a..e3de547 100644
>>> --- a/libavcodec/libvpxenc.c
>>> +++ b/libavcodec/libvpxenc.c
>>> @@ -1057,6 +1057,135 @@ static int queue_frames(AVCodecContext *avctx,
>>> AVPacket *pkt_out)
>>>   return size;
>>>   }
>>>
>>> +static int vp8_encode_set_roi(AVCodecContext *avctx, const AVFrame
>>> *frame)
>>> +{
>>> +    /* range of vpx_roi_map_t.delta_q[i] is [-63, 63] */
>>> +#define MAX_DELTA_Q 63
>>> +
>>> +    AVRegionOfInterest *roi = NULL;
>>> +    vpx_roi_map_t roi_map;
>>> +    AVFrameSideData *sd = av_frame_get_side_data(frame,
>>> AV_FRAME_DATA_REGIONS_OF_INTEREST);
>>> +    VPxContext *ctx = avctx->priv_data;
>>> +    vpx_active_map_t active_map = { 0, 0, 0 };
>>> +    int segment_id;
>>> +    int zero_delta_q = 0;
>>> +
>>> +    /* record the mapping from delta_q to "segment id + 1".
>>> + * delta_q is shift with MAX_DELTA_Q, and so the range is [0,
>>> 2*MAX_DELTA_Q].
>>> + * add 1 to segment id, so no mapping if the value of array element is 
>>> zero.
>>> + */
>>> +    int segment_mapping[2 * MAX_DELTA_Q + 1] = {0};
>>> +
>>> +    active_map.rows = (frame->height + 15) / 16;
>>> +    active_map.cols = (frame->width  + 15) / 16;
>>> +
>>> +    if (!sd) {
>>> +    if (vpx_codec_control(&ctx->encoder, VP8E_SET_ACTIVEMAP,
>>> &active_map)) {
>>> +    log_encoder_error(avctx, "Failed to set VP8E_SET_ACTIVEMAP 
>>> codec
>>> control.\n");
>>> +    return AVERROR_INVALIDDATA;
>>> +    }
>>> +    return 0;
>>> +    }
>>> +
>>> +    active_map.active_map = av_malloc(active_map.rows * active_map.cols);
>>> +    if (!active_map.active_map) {
>>> +    av_log(avctx, AV_LOG_ERROR,
>>> +   "active_map alloc (%d bytes) failed.\n",
>>> +   active_map.rows * active_map.cols);
>>> +    return AVERROR(ENOMEM);
>>> +    }
>>> +    memset(active_map.active_map, 1, active_map.rows * active_map.cols);
>>> +
>>> +    /* segment id 0 in roi_map is reserved for the areas not covered by
>>> AVRegionOfInterest.
>>> + * segment id 0 in roi_map is also for the areas with
>>> AVRegionOfInterest.qoffset near 0.
>>> + */
>>> +    segment_id = 0;
>>> +    segment_mapping[zero_delta_q + MAX_DELTA_Q] = segment_id + 1;
>>> +    segment_id++;
>>> +    memset(&roi_map, 0, sizeof(roi_map));
>>> +    roi_map.delta_q[segment_id] = zero_delta_q;
>>> +
>>> +    roi_map.rows = active_map.rows;
>>> +    roi_map.cols = active_map.cols;
>>> +    roi_map.roi_map = av_mallocz(roi_map.rows * roi_map.cols);
>>> +    if (!roi_map.roi_map) {
>>> +    av_free(active_map.active_map);
>>> +    av_log(avctx, AV_LOG_ERROR,
>>> +   "roi_map alloc (%d bytes) failed.\n",
>>> +   roi_map.rows * roi_map.cols);
>>> +    return AVERROR(ENOMEM);
>>> +    }
>>> +
>>> +    roi = (AVRegionOfInterest*)sd->data;
>>> +    while ((uint8_t*)roi < sd->data + sd->size) {
>>> +    int qoffset;
>>> +    int mapping_index;
>>> +    int mapping_value;
>>> +    int starty = FFMIN(roi_map.rows, roi->top / 16);
>>> +    int endy   = FFMIN(roi_map.rows, (roi->bottom + 15) / 16);
>>> +    int startx = FFMIN(roi_map.cols, roi->left / 16);
>>> +    int endx   = FFMIN(roi_map.cols, (roi->right + 15) / 16);
>>> +
>>> +    if (roi->self_size == 0) {
>>> +    av_free(active_map.active_map);
>>> +    av_free(roi_map.roi_map);
>>> +    av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size must be
>>> set to sizeof(AVRegionOfInterest).\n");
>>> +    return AVERROR(EINVAL);
>>> +    }
>>> +
>>> +    if (roi->qoffset.den == 0) {
>>> +    av_free(active_map.active_map);
>>> +    av_free(roi_map.roi_map);
>>> +    av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.qoffset.den must
>>> not be zero.\n");
>>> +    return AVERROR(EINVAL);
>>> +    }
>>> +
>>> +    qoffset = (int)(roi->qoffset.num * 1.0f / roi->qoffset.den *
>>> MAX_DELTA_Q);
>>> +    qoffset = av_clip(qoffset, -MAX_DELTA_Q, MAX_DELTA_Q);
>>> +
>>> +    mapping_index = qoffset + MAX_DELTA_Q;
>>> +    if (!segment_mapping[mapping_index]) {
>>> +    if (segment_id > 3) {
>>> +    av_log(ctx, AV_LOG_WARNING,
>>> +   "ROI only support 4 segments (and segment 0 is 
>>> reserved for
>>> non-ROIs), skipping this one.\n");
>>> +    

[FFmpeg-devel] [PATCH 4/5] vaapi_encode: Add ROI support

[Mark Thompson]

---
 libavcodec/vaapi_encode.c   | 129 
 libavcodec/vaapi_encode.h   |   9 +++
 libavcodec/vaapi_encode_h264.c  |   2 +
 libavcodec/vaapi_encode_h265.c  |   2 +
 libavcodec/vaapi_encode_mpeg2.c |   2 +
 libavcodec/vaapi_encode_vp8.c   |   2 +
 libavcodec/vaapi_encode_vp9.c   |   2 +
 7 files changed, 148 insertions(+)

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 2dda451882..1865006c1a 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -143,6 +143,7 @@ static int vaapi_encode_issue(AVCodecContext *avctx,
 int err, i;
 char data[MAX_PARAM_BUFFER_SIZE];
 size_t bit_len;
+AVFrameSideData *sd;
 
 av_log(avctx, AV_LOG_DEBUG, "Issuing encode for pic %"PRId64"/%"PRId64" "
"as type %s.\n", pic->display_order, pic->encode_order,
@@ -412,6 +413,92 @@ static int vaapi_encode_issue(AVCodecContext *avctx,
 }
 }
 
+sd = av_frame_get_side_data(pic->input_image,
+AV_FRAME_DATA_REGIONS_OF_INTEREST);
+
+#if VA_CHECK_VERSION(1, 0, 0)
+if (sd && ctx->roi_allowed) {
+const AVRegionOfInterest *roi;
+int nb_roi;
+VAEncMiscParameterBuffer param_misc = {
+.type = VAEncMiscParameterTypeROI,
+};
+VAEncMiscParameterBufferROI param_roi;
+// VAAPI requires the second structure to immediately follow the
+// first in the parameter buffer, but they have different natural
+// alignment on 64-bit architectures (4 vs. 8, since the ROI
+// structure contains a pointer).  This means we can't make a
+// single working structure, nor can we make the buffer
+// separately and map it because dereferencing the second pointer
+// would be undefined.  Therefore, construct the two parts
+// separately and combine them into a single character buffer
+// before uploading.
+char param_buffer[sizeof(param_misc) + sizeof(param_roi)];
+int i, v;
+
+roi = (const AVRegionOfInterest*)sd->data;
+av_assert0(roi->self_size && sd->size % roi->self_size == 0);
+nb_roi = sd->size / roi->self_size;
+if (nb_roi > ctx->roi_regions) {
+if (!ctx->roi_warned) {
+av_log(avctx, AV_LOG_WARNING, "More ROIs set than "
+   "supported by driver (%d > %d).\n",
+   nb_roi, ctx->roi_regions);
+ctx->roi_warned = 1;
+}
+nb_roi = ctx->roi_regions;
+}
+
+pic->roi = av_mallocz_array(nb_roi, sizeof(*pic->roi));
+if (!pic->roi) {
+err = AVERROR(ENOMEM);
+goto fail;
+}
+for (i = 0; i < nb_roi; i++) {
+roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
+
+v = roi->qoffset.num * ctx->roi_quant_range / roi->qoffset.den;
+av_log(avctx, AV_LOG_DEBUG, "ROI region: (%d-%d,%d-%d) -> %+d.\n",
+   roi->left, avctx->width - roi->right,
+   roi->top, avctx->height - roi->bottom, v);
+
+pic->roi[i] = (VAEncROI) {
+.roi_rectangle = {
+.x  = roi->left,
+.y  = roi->top,
+.width  = avctx->width  - roi->right - roi->left,
+.height = avctx->height - roi->bottom - roi->top,
+},
+.roi_value = av_clip_c(v, INT8_MIN, INT8_MAX),
+};
+}
+
+param_roi = (VAEncMiscParameterBufferROI) {
+.num_roi  = nb_roi,
+.max_delta_qp = INT8_MAX,
+.min_delta_qp = 0,
+.roi  = pic->roi,
+.roi_flags.bits.roi_value_is_qp_delta = 1,
+};
+
+memcpy(param_buffer, ¶m_misc, sizeof(param_misc));
+memcpy(param_buffer + sizeof(param_misc),
+   ¶m_roi, sizeof(param_roi));
+
+err = vaapi_encode_make_param_buffer(avctx, pic,
+ VAEncMiscParameterBufferType,
+ param_buffer,
+ sizeof(param_buffer));
+if (err < 0)
+goto fail;
+} else
+#endif
+if (sd && !ctx->roi_warned) {
+av_log(avctx, AV_LOG_WARNING, "ROI side data on input "
+   "frames ignored due to lack of driver support.\n");
+ctx->roi_warned = 1;
+}
+
 vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,
  pic->input_surface);
 if (vas != VA_STATUS_SUCCESS) {
@@ -477,6 +564,7 @@ fail_at_end:
 av_freep(&pic->codec_picture_params);
 av_freep(&pic->param_buffers);
 av_freep(&pic->slices);
+av_freep(&pic->roi);
 av_frame_free(&pic->recon_image);
 av_buffer_unref(&pic->output_buffer_ref);
 pic->output_buffer = VA_INVALID_ID;
@@ -611,6 +699,7 @@ static int vaapi_encode

[FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to match documentation

[Mark Thompson]

Fix the quantisation offset - use the whole range, and don't change the
offset size based on bit depth.

Use correct bottom/right edge locations (they are offsets from bottom/right,
not from top/left).

Iterate the list in reverse order.  The regions are in order of decreasing
importance, so the most important must be applied last.
---
 libavcodec/libx264.c | 50 
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index a3493f393d..475719021e 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -285,16 +285,18 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, 
const AVFrame *frame,
 int nnal, i, ret;
 x264_picture_t pic_out = {0};
 int pict_type;
+int bit_depth;
 int64_t *out_opaque;
 AVFrameSideData *sd;
 
 x264_picture_init( &x4->pic );
 x4->pic.img.i_csp   = x4->params.i_csp;
 #if X264_BUILD >= 153
-if (x4->params.i_bitdepth > 8)
+bit_depth = x4->params.i_bitdepth;
 #else
-if (x264_bit_depth > 8)
+bit_depth = x264_bit_depth;
 #endif
+if (bit_depth > 8)
 x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;
 x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
 
@@ -359,45 +361,47 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, 
const AVFrame *frame,
 if (frame->interlaced_frame == 0) {
 int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
 int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
+int qp_range = 51 + 6 * (bit_depth - 8);
 int nb_rois;
-AVRegionOfInterest* roi;
-float* qoffsets;
+const 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);
+roi = (const AVRegionOfInterest*)sd->data;
+if (!roi->self_size || sd->size % roi->self_size != 0) {
+av_log(ctx, AV_LOG_ERROR, "Invalid 
AVRegionOfInterest.self_size.\n");
+return AVERROR(EINVAL);
+}
+nb_rois = sd->size / roi->self_size;
+
+// This list must be iterated in reverse because regions 
are
+// defined in order of decreasing importance.
+for (int i = nb_rois - 1; i >= 0; i--) {
+int startx, endx, starty, endy;
 float qoffset;
 
+roi = (const AVRegionOfInterest*)(sd->data + 
roi->self_size * i);
+
+starty = av_clip(roi->top / MB_SIZE, 0, mby);
+endy   = av_clip((frame->height - roi->bottom + 
MB_SIZE - 1) / MB_SIZE, 0, mby);
+startx = av_clip(roi->left / MB_SIZE, 0, mbx);
+endx   = av_clip((frame->width - roi->right + MB_SIZE 
- 1) / MB_SIZE, 0, mbx);
+
 if (roi->qoffset.den == 0) {
 av_free(qoffsets);
-av_log(ctx, AV_LOG_ERROR, 
"AVRegionOfInterest.qoffset.den should not be zero.\n");
+av_log(ctx, AV_LOG_ERROR, 
"AVRegionOfInterest.qoffset.den must 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;
+qoffset = av_clipf(qoffset * qp_range, -qp_range, 
+qp_range);
 
 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);
-}
-

[FFmpeg-devel] [PATCH 3/5] libx265: Update ROI behaviour to match documentation

[Mark Thompson]

Equivalent to the previous patch for libx264.
---
 libavcodec/libx265.c | 41 -
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 98415366da..af1a2b2287 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -296,27 +296,33 @@ static av_cold int libx265_encode_set_roi(libx265Context 
*ctx, const AVFrame *fr
 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 qp_range = 51 + 6 * (pic->bitDepth - 8);
 int nb_rois;
-AVRegionOfInterest *roi;
+const 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);
+roi = (const AVRegionOfInterest*)sd->data;
+if (!roi->self_size || sd->size % roi->self_size != 0) {
+av_log(ctx, AV_LOG_ERROR, "Invalid 
AVRegionOfInterest.self_size.\n");
+return AVERROR(EINVAL);
+}
+nb_rois = sd->size / roi->self_size;
+
+// This list must be iterated in reverse because regions are
+// defined in order of decreasing importance.
+for (int i = nb_rois - 1; i >= 0; i--) {
+int startx, endx, starty, endy;
 float qoffset;
 
-if (roi->self_size == 0) {
-av_free(qoffsets);
-av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size 
must be set to sizeof(AVRegionOfInterest).\n");
-return AVERROR(EINVAL);
-}
+roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * 
i);
+
+starty = av_clip(roi->top / mb_size, 0, mby);
+endy   = av_clip((frame->height - roi->bottom + mb_size - 1) / 
mb_size, 0, mby);
+startx = av_clip(roi->left / mb_size, 0, mbx);
+endx   = av_clip((frame->width - roi->right + mb_size - 1) / 
mb_size, 0, mbx);
 
 if (roi->qoffset.den == 0) {
 av_free(qoffsets);
@@ -324,18 +330,11 @@ static av_cold int libx265_encode_set_roi(libx265Context 
*ctx, const AVFrame *fr
 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;
+qoffset = av_clipf(qoffset * qp_range, -qp_range, +qp_range);
 
 for (int y = starty; y < endy; y++)
 for (int x = startx; x < endx; x++)
 qoffsets[x + y*mbx] = qoffset;
-
-roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
 }
 
 pic->quantOffsets = qoffsets;
-- 
2.19.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/5] lavu/frame: Expand ROI documentation

[Mark Thompson]

Clarify and add examples for the behaviour of the quantisation offset,
and define how multiple ranges should be handled.
---
 libavutil/frame.h | 62 +++
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/libavutil/frame.h b/libavutil/frame.h
index 8aa3e88367..2cd6c33a90 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -207,31 +207,61 @@ typedef struct AVFrameSideData {
 } AVFrameSideData;
 
 /**
- * Structure to hold Region Of Interest.
+ * Structure describing a single Region Of Interest.
  *
- * self_size specifies the size of this data structure. This value
- * should be set to sizeof(AVRegionOfInterest). EINVAL is returned if 
self_size is zero.
+ * When multiple regions are defined in a single side-data block, they
+ * should be ordered from most to least important - some encoders are only
+ * capable of supporting a limited number of distinct regions, so will have
+ * to truncate the list.
  *
- * Number of pixels to discard from the top/bottom/left/right border of
- * the frame to obtain the region of interest of the frame.
- * They are encoder dependent and will be extended internally
- * if the codec requires an alignment.
- * If the regions overlap, the last value in the list will be used.
- *
- * qoffset is quant offset, and base rule here:
- * returns EINVAL if AVRational.den is zero.
- * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out of range.
- * 0 means no picture quality change,
- * negative offset asks for better quality (and the best with value -1.0),
- * positive offset asks for worse quality (and the worst with value 1.0).
- * How to explain/implement the different quilaity requirement is encoder 
dependent.
+ * When overlapping regions are defined, the first region containing a given
+ * area of the frame applies.
  */
 typedef struct AVRegionOfInterest {
+/**
+ * Must be set to the size of this data structure (that is,
+ * sizeof(AVRegionOfInterest)).
+ */
 uint32_t self_size;
+/**
+ * Number of pixels to discard from the top/bottom/left/right border of
+ * the frame to obtain the region of interest.
+ *
+ * The constraints on a region are encoder dependent, so the region
+ * actually affected may be slightly larger for alignment or other
+ * reasons.
+ */
 int top;
 int bottom;
 int left;
 int right;
+/**
+ * Quantisation offset.
+ *
+ * Must be in the range -1 to +1.  A value of zero indicates no quality
+ * change.  A negative value asks for better quality (less quantisation),
+ * while a positive value asks for worse quality (greater quantisation).
+ *
+ * The range is calibrated so that the extreme values indicate the
+ * largest possible offset - if the rest of the frame is encoded with the
+ * worst possible quality, an offset of -1 indicates that this region
+ * should be encoded with the best possible quality anyway.  Intermediate
+ * values are then interpolated in some codec-dependent way.
+ *
+ * For example, in 10-bit H.264 the quantisation parameter varies between
+ * -12 and 51.  A typical qoffset value of -1/10 therefore indicates that
+ * this region should be encoded with a QP around one-tenth of the full
+ * range better than the rest of the frame.  So, if most of the frame
+ * were to be encoded with a QP of around 30, this region would get a QP
+ * of around 24 (an offset of approximately -1/10 * (51 - -12) = -6.3).
+ *
+ * The extreme qoffset values (-1 and +1) therefore indicate that the
+ * region should be encoded with the best/worst possible quality
+ * regardless of the treatment of the rest of the frame - e.g. if qoffset
+ * is -1 in the 10-bit H.264 example, this region should use a QP of -12
+ * independent of the rest of the frame (which could be good quality with
+ * QP -4, or terrible quality with QP 50, or any other value).
+ */
 AVRational qoffset;
 } AVRegionOfInterest;
 
-- 
2.19.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat/westwood_aud: Adds PCM format demux.

[Aidan R]

PCM format AUD files are found in Westwood's Blade Runner game.
---
 libavformat/westwood_aud.c | 80 --
 1 file changed, 63 insertions(+), 17 deletions(-)

diff --git a/libavformat/westwood_aud.c b/libavformat/westwood_aud.c
index 9c2d35cb8a..5d7e827bc1 100644
--- a/libavformat/westwood_aud.c
+++ b/libavformat/westwood_aud.c
@@ -41,6 +41,12 @@
 #define AUD_HEADER_SIZE 12
 #define AUD_CHUNK_PREAMBLE_SIZE 8
 #define AUD_CHUNK_SIGNATURE 0xDEAF
+#define AUD_PCM_CHUNK_SIZE 2048 /* arbitrary size to pull out of PCM data*/
+
+typedef struct AUDDemuxContext {
+int size;
+int pos;
+} AUDDemuxContext;
 
 static int wsaud_probe(AVProbeData *p)
 {
@@ -50,10 +56,10 @@ static int wsaud_probe(AVProbeData *p)
  * so perform sanity checks on various header parameters:
  *   8000 <= sample rate (16 bits) <= 48000  ==> 40001 acceptable numbers
  *   flags <= 0x03 (2 LSBs are used) ==> 4 acceptable numbers
- *   compression type (8 bits) = 1 or 99 ==> 2 acceptable numbers
+ *   compression type (8 bits) = 0, 1 or 99  ==> 3 acceptable numbers
  *   first audio chunk signature (32 bits)   ==> 1 acceptable number
- * The number space contains 2^64 numbers. There are 40001 * 4 * 2 * 1 =
- * 320008 acceptable number combinations.
+ * The number space contains 2^64 numbers. There are 40001 * 4 * 3 * 1 =
+ * 480012 acceptable number combinations.
  */
 
 if (p->buf_size < AUD_HEADER_SIZE + AUD_CHUNK_PREAMBLE_SIZE)
@@ -69,13 +75,22 @@ static int wsaud_probe(AVProbeData *p)
 if (p->buf[10] & 0xFC)
 return 0;
 
-if (p->buf[11] != 99 && p->buf[11] != 1)
+/* valid format values are 99 == adpcm, 1 == snd1 and 0 == pcm */
+if (p->buf[11] != 99 && p->buf[11] != 1 && p->buf[11] != 0)
 return 0;
 
-/* read ahead to the first audio chunk and validate the first header 
signature */
-if (AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
+/* read ahead to the first audio chunk and validate the first header
+ * signature pcm format does not use a chunk format, so don't check
+ * for that codec */
+if (p->buf[11] != 0 && AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
 return 0;
 
+/* if we have pcm format then compressed size should equal
+ * uncompressed size */
+if (p->buf[11] == 0 && AV_RL32(&p->buf[2]) != AV_RL32(&p->buf[6]))  {
+return 0;
+}
+
 /* return 1/2 certainty since this file check is a little sketchy */
 return AVPROBE_SCORE_EXTENSION;
 }
@@ -83,16 +98,20 @@ static int wsaud_probe(AVProbeData *p)
 static int wsaud_read_header(AVFormatContext *s)
 {
 AVIOContext *pb = s->pb;
+AUDDemuxContext *aud = s->priv_data;
 AVStream *st;
 unsigned char header[AUD_HEADER_SIZE];
-int sample_rate, channels, codec;
+int sample_rate, channels, codec, bits_per_sample;
 
 if (avio_read(pb, header, AUD_HEADER_SIZE) != AUD_HEADER_SIZE)
 return AVERROR(EIO);
 
 sample_rate = AV_RL16(&header[0]);
 channels= (header[10] & 0x1) + 1;
+bits_per_sample = (header[10] & 0x2) ? 16 : 8;
 codec   = header[11];
+aud->size   = AV_RL32(&header[2]);
+aud->pos= 0; /* used to track position in a PCM stream */
 
 /* initialize the audio decoder stream */
 st = avformat_new_stream(s, NULL);
@@ -100,6 +119,15 @@ static int wsaud_read_header(AVFormatContext *s)
 return AVERROR(ENOMEM);
 
 switch (codec) {
+case  0:
+if (bits_per_sample == 8) {
+st->codecpar->codec_id = AV_CODEC_ID_PCM_U8;
+} else {
+st->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE;
+}
+st->codecpar->bits_per_coded_sample = bits_per_sample;
+st->codecpar->bit_rate = channels * sample_rate * bits_per_sample;
+break;
 case  1:
 if (channels != 1) {
 avpriv_request_sample(s, "Stereo WS-SND1");
@@ -130,20 +158,24 @@ static int wsaud_read_packet(AVFormatContext *s,
  AVPacket *pkt)
 {
 AVIOContext *pb = s->pb;
+AUDDemuxContext *aud = s->priv_data;
 unsigned char preamble[AUD_CHUNK_PREAMBLE_SIZE];
-unsigned int chunk_size;
+unsigned int chunk_size, bytes_per_sample;
 int ret = 0;
 AVStream *st = s->streams[0];
 
-if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
-AUD_CHUNK_PREAMBLE_SIZE)
-return AVERROR(EIO);
+/* AUD files don't store PCM audio in chunks */
+if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) {
+if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
+AUD_CHUNK_PREAMBLE_SIZE)
+return AVERROR(EIO);
 
-/* validate the chunk */
-if (AV_RL32(&preamble[4]) != AUD_CHUNK_SIGNATURE)
-return AVERROR_INVALIDDATA;
+/* validate the chunk */
+if (AV_RL32(&preamble[4]) != AUD_CHUNK_SIGNATURE)
+return AVERROR_INVALIDDATA;
 
-chunk_size = AV_RL16(&preamble[0]);

Re: [FFmpeg-devel] [PATCH 5/5] lavfi: addroi filter

[Moritz Barsnick]

On Wed, Feb 27, 2019 at 22:00:23 +, Mark Thompson wrote:
> +static const AVOption addroi_options[] = {
> +{ "top","Region distance from top edge of frame",
> +  OFFSET(region_str[TOP]),AV_OPT_TYPE_STRING, { .str = "0" }, .flags 
> = FLAGS },
> +{ "bottom", "Region distance from bottom edge of frame",
> +  OFFSET(region_str[BOTTOM]), AV_OPT_TYPE_STRING, { .str = "0" }, .flags 
> = FLAGS },
> +{ "left",   "Region distance from left edge of frame",
> +  OFFSET(region_str[LEFT]),   AV_OPT_TYPE_STRING, { .str = "0" }, .flags 
> = FLAGS },
> +{ "right",  "Region distance from right edge of frame",
> +  OFFSET(region_str[RIGHT]),  AV_OPT_TYPE_STRING, { .str = "0" }, .flags 
> = FLAGS },

My comment may not apply here, but I sometimes wish all rectangle
defining filters had compatible options.

I.e. drawbox and crop and delogo (which I use interchangably - e.g.
first the one to visually determine the other's dimensions) aren't
compatible with respect to order of unnamed options, and to names of
variables in their expression (w, iw, ow, ...).

Can ROIs not also be defined as x, y, w, h?

Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/scpr: Fix use of uninitialized variable

[Michael Niedermayer]

Fixes: Undefined shift
Fixes: 
12911/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-5677102915911680

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/scpr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
index cf511f83cc..750cf59fe4 100644
--- a/libavcodec/scpr.c
+++ b/libavcodec/scpr.c
@@ -372,7 +372,7 @@ static int decompress_p(AVCodecContext *avctx,
 {
 SCPRContext *s = avctx->priv_data;
 GetByteContext *gb = &s->gb;
-int ret, temp, min, max, x, y, cx = 0, cx1 = 0;
+int ret, temp = 0, min, max, x, y, cx = 0, cx1 = 0;
 int backstep = linesize - avctx->width;
 
 if (bytestream2_get_byte(gb) == 0)
-- 
2.21.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v3 1/2] lavfi/vaapi: Improve support for colour properties

[Mark Thompson]

Attempts to pick the set of supported colour properties best matching the
input.  Output is then set with the same values, except for the colour
matrix which may change when converting between RGB and YUV.
---
Not much change since the version sent two months ago - rebased, and the 
transpose filter updated to match the others.


 libavfilter/vaapi_vpp.c| 273 -
 libavfilter/vaapi_vpp.h|   5 +-
 libavfilter/vf_deinterlace_vaapi.c |  16 +-
 libavfilter/vf_misc_vaapi.c|  13 +-
 libavfilter/vf_procamp_vaapi.c |  13 +-
 libavfilter/vf_scale_vaapi.c   |  12 +-
 libavfilter/vf_transpose_vaapi.c   |  15 +-
 7 files changed, 309 insertions(+), 38 deletions(-)

diff --git a/libavfilter/vaapi_vpp.c b/libavfilter/vaapi_vpp.c
index c5bbc3b85b..f4ee622a2b 100644
--- a/libavfilter/vaapi_vpp.c
+++ b/libavfilter/vaapi_vpp.c
@@ -234,18 +234,273 @@ fail:
 return err;
 }
 
-int ff_vaapi_vpp_colour_standard(enum AVColorSpace av_cs)
+typedef struct VAAPIColourProperties {
+VAProcColorStandardType va_color_standard;
+
+enum AVColorPrimaries color_primaries;
+enum AVColorTransferCharacteristic color_trc;
+enum AVColorSpace colorspace;
+
+uint8_t va_chroma_sample_location;
+uint8_t va_color_range;
+
+enum AVColorRange color_range;
+enum AVChromaLocation chroma_sample_location;
+} VAAPIColourProperties;
+
+static const VAAPIColourProperties*
+vaapi_vpp_find_colour_props(VAProcColorStandardType vacs)
+{
+static const VAAPIColourProperties cs_map[] = {
+{ VAProcColorStandardBT601,   5,  6,  5 },
+{ VAProcColorStandardBT601,   6,  6,  6 },
+{ VAProcColorStandardBT709,   1,  1,  1 },
+{ VAProcColorStandardBT470M,  4,  4,  4 },
+{ VAProcColorStandardBT470BG, 5,  5,  5 },
+{ VAProcColorStandardSMPTE170M,   6,  6,  6 },
+{ VAProcColorStandardSMPTE240M,   7,  7,  7 },
+{ VAProcColorStandardGenericFilm, 8,  1,  1 },
+#if VA_CHECK_VERSION(1, 1, 0)
+{ VAProcColorStandardSRGB,1, 13,  0 },
+{ VAProcColorStandardXVYCC601,1, 11,  5 },
+{ VAProcColorStandardXVYCC709,1, 11,  1 },
+{ VAProcColorStandardBT2020,  9, 14,  9 },
+#endif
+};
+int i;
+for (i = 0; i < FF_ARRAY_ELEMS(cs_map); i++) {
+if (vacs == cs_map[i].va_color_standard)
+return &cs_map[i];
+}
+return NULL;
+}
+
+static void vaapi_vpp_fill_colour_standard(VAAPIColourProperties *props,
+   VAProcColorStandardType *vacs,
+   int nb_vacs)
+{
+const VAAPIColourProperties *t;
+int i, k, score, best_score, worst_score;
+
+// If the driver supports explicit use of the standard values then just
+// use them and avoid doing any mapping.  (The driver may not support
+// some particular code point, but it still has enough information to
+// make a better fallback choice than we do in that case.)
+#if VA_CHECK_VERSION(1, 1, 0)
+for (i = 0; i < nb_vacs; i++) {
+if (vacs[i] == VAProcColorStandardExplicit) {
+props->va_color_standard = VAProcColorStandardExplicit;
+return;
+}
+}
+#endif
+
+// Give scores to the possible options and choose the lowest one.
+// An exact match will score zero and therefore always be chosen, as
+// will a partial match where all unmatched elements are explicitly
+// unspecified.  (If all elements are unspecified this will use the
+// first available value.)  If no options match at all then just
+// pass "none" to the driver and let it make its own choice.
+best_score = -1;
+worst_score = 4 * (props->colorspace != AVCOL_SPC_UNSPECIFIED) +
+  2 * (props->color_trc != AVCOL_TRC_UNSPECIFIED) +
+  (props->color_primaries != AVCOL_PRI_UNSPECIFIED);
+for (k = 0; k < 2; k++) {
+for (i = 0; i < nb_vacs; i++) {
+t = vaapi_vpp_find_colour_props(vacs[i]);
+if (!t)
+continue;
+
+score = 0;
+if (props->colorspace != AVCOL_SPC_UNSPECIFIED &&
+props->colorspace != AVCOL_SPC_RGB)
+score += 4 * (props->colorspace != t->colorspace);
+if (props->color_trc != AVCOL_TRC_UNSPECIFIED)
+score += 2 * (props->color_trc != t->color_trc);
+if (props->color_primaries != AVCOL_PRI_UNSPECIFIED)
+score += (props->color_primaries != t->color_primaries);
+
+if (k == 0)  {
+// Only include things which matched something.
+if ((worst_score == 0 || score < worst_score) &&
+(best_score == -1 || score < best_score))
+best_score = score;
+} else {
+if (score == best_score) {
+props->va_color_standard = t->va_color_standard;
+ 

[FFmpeg-devel] [PATCH v3 2/2] vf_scale_vaapi: Add options to configure output colour properties

[Mark Thompson]

The "out_color_matrix" and "out_range" properties match the same options
in vf_scale; the others attempt to follow the same pattern.
---
 libavfilter/vf_scale_vaapi.c | 70 
 1 file changed, 70 insertions(+)

diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
index b56217c56d..24d72ed82a 100644
--- a/libavfilter/vf_scale_vaapi.c
+++ b/libavfilter/vf_scale_vaapi.c
@@ -39,6 +39,17 @@ typedef struct ScaleVAAPIContext {
 
 char *w_expr;  // width expression string
 char *h_expr;  // height expression string
+
+char *colour_primaries_string;
+char *colour_transfer_string;
+char *colour_matrix_string;
+int colour_range;
+char *chroma_location_string;
+
+enum AVColorPrimaries colour_primaries;
+enum AVColorTransferCharacteristic colour_transfer;
+enum AVColorSpace colour_matrix;
+enum AVChromaLocation chroma_location;
 } ScaleVAAPIContext;
 
 static const char *scale_vaapi_mode_name(int mode)
@@ -116,6 +127,17 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, 
AVFrame *input_frame)
 if (err < 0)
 goto fail;
 
+if (ctx->colour_primaries != AVCOL_PRI_UNSPECIFIED)
+output_frame->color_primaries = ctx->colour_primaries;
+if (ctx->colour_transfer != AVCOL_TRC_UNSPECIFIED)
+output_frame->color_trc = ctx->colour_transfer;
+if (ctx->colour_matrix != AVCOL_SPC_UNSPECIFIED)
+output_frame->colorspace = ctx->colour_matrix;
+if (ctx->colour_range != AVCOL_RANGE_UNSPECIFIED)
+output_frame->color_range = ctx->colour_range;
+if (ctx->chroma_location != AVCHROMA_LOC_UNSPECIFIED)
+output_frame->chroma_location = ctx->chroma_location;
+
 output_surface = (VASurfaceID)(uintptr_t)output_frame->data[3];
 av_log(avctx, AV_LOG_DEBUG, "Using surface %#x for scale output.\n",
output_surface);
@@ -183,6 +205,24 @@ static av_cold int scale_vaapi_init(AVFilterContext *avctx)
 vpp_ctx->output_format = AV_PIX_FMT_NONE;
 }
 
+#define STRING_OPTION(var_name, func_name, default_value) do { \
+if (ctx->var_name ## _string) { \
+int var = av_ ## func_name ## _from_name(ctx->var_name ## 
_string); \
+if (var < 0) { \
+av_log(avctx, AV_LOG_ERROR, "Invalid %s.\n", #var_name); \
+return AVERROR(EINVAL); \
+} \
+ctx->var_name = var; \
+} else { \
+ctx->var_name = default_value; \
+} \
+} while (0)
+
+STRING_OPTION(colour_primaries, color_primaries, AVCOL_PRI_UNSPECIFIED);
+STRING_OPTION(colour_transfer,  color_transfer,  AVCOL_TRC_UNSPECIFIED);
+STRING_OPTION(colour_matrix,color_space, AVCOL_SPC_UNSPECIFIED);
+STRING_OPTION(chroma_location,  chroma_location, AVCHROMA_LOC_UNSPECIFIED);
+
 return 0;
 }
 
@@ -206,6 +246,36 @@ static const AVOption scale_vaapi_options[] = {
   0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_HQ }, 0, 0, FLAGS,  
"mode" },
 { "nl_anamorphic", "Use nolinear anamorphic scaling algorithm",
   0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_NL_ANAMORPHIC }, 0, 
0, FLAGS,  "mode" },
+
+// These colour properties match the ones of the same name in vf_scale.
+{ "out_color_matrix", "Output colour matrix coefficient set",
+  OFFSET(colour_matrix_string), AV_OPT_TYPE_STRING, { .str = NULL }, 
.flags = FLAGS },
+{ "out_range", "Output colour range",
+  OFFSET(colour_range), AV_OPT_TYPE_INT, { .i64 = AVCOL_RANGE_UNSPECIFIED 
},
+  AVCOL_RANGE_UNSPECIFIED, AVCOL_RANGE_JPEG, FLAGS, "range" },
+{ "full","Full range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_JPEG }, 0, 0, FLAGS, "range" 
},
+{ "limited", "Limited range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_MPEG }, 0, 0, FLAGS, "range" 
},
+{ "jpeg","Full range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_JPEG }, 0, 0, FLAGS, "range" 
},
+{ "mpeg","Limited range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_MPEG }, 0, 0, FLAGS, "range" 
},
+{ "tv",  "Limited range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_MPEG }, 0, 0, FLAGS, "range" 
},
+{ "pc",  "Full range",
+  0, AV_OPT_TYPE_CONST, { .i64 = AVCOL_RANGE_JPEG }, 0, 0, FLAGS, "range" 
},
+// These colour properties are new here.
+{ "out_color_primaries", "Output colour primaries",
+  OFFSET(colour_primaries_string), AV_OPT_TYPE_STRING,
+  { .str = NULL }, .flags = FLAGS },
+{ "out_color_transfer", "Output colour transfer characteristics",
+  OFFSET(colour_transfer_string),  AV_OPT_TYPE_STRING,
+  { .str = NULL }, .flags = FLAGS },
+{ "out_chroma_location", "Output chroma sample location",
+  OFFSET(chroma_location_string),  AV_OPT_TYPE_STRING,
+  { .str = NULL }, .flags = FLAGS },
+
 { NULL },
 };
 
-- 
2.19.2

___
ffmpeg-devel mailing list
ffmpeg-d

Re: [FFmpeg-devel] [PATCH 5/5] lavfi: addroi filter

[Mark Thompson]

On 27/02/2019 23:42, Moritz Barsnick wrote:
> On Wed, Feb 27, 2019 at 22:00:23 +, Mark Thompson wrote:
>> +static const AVOption addroi_options[] = {
>> +{ "top","Region distance from top edge of frame",
>> +  OFFSET(region_str[TOP]),AV_OPT_TYPE_STRING, { .str = "0" }, 
>> .flags = FLAGS },
>> +{ "bottom", "Region distance from bottom edge of frame",
>> +  OFFSET(region_str[BOTTOM]), AV_OPT_TYPE_STRING, { .str = "0" }, 
>> .flags = FLAGS },
>> +{ "left",   "Region distance from left edge of frame",
>> +  OFFSET(region_str[LEFT]),   AV_OPT_TYPE_STRING, { .str = "0" }, 
>> .flags = FLAGS },
>> +{ "right",  "Region distance from right edge of frame",
>> +  OFFSET(region_str[RIGHT]),  AV_OPT_TYPE_STRING, { .str = "0" }, 
>> .flags = FLAGS },
> 
> My comment may not apply here, but I sometimes wish all rectangle
> defining filters had compatible options.
> 
> I.e. drawbox and crop and delogo (which I use interchangably - e.g.
> first the one to visually determine the other's dimensions) aren't
> compatible with respect to order of unnamed options, and to names of
> variables in their expression (w, iw, ow, ...).
> 
> Can ROIs not also be defined as x, y, w, h?

Sure, it would be easy to change if people prefer that.  This was chosen pretty 
much entirely because it matches the structure definition (where I think the 
symmetric style is indeed cleanest?  I guess you could have the same debate 
there if you like...).  I certainly was making use of the order of options when 
using it, though - e.g. "addroi=h/4:h/4:w/4:w/4:-1/5" to highlight the central 
quarter of the frame.

Wrt variables, I don't have 'i'/'o' prefixed names because the input and output 
are the same frame.  Would "iw"/"ih" be better, or would that just be more 
confusing?

Thanks,

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to match documentation

[Guo, Yejun]

> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, February 28, 2019 6:00 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to
> match documentation
> 
> Fix the quantisation offset - use the whole range, and don't change the
> offset size based on bit depth.
> 
> Use correct bottom/right edge locations (they are offsets from bottom/right,
> not from top/left).
> 
> Iterate the list in reverse order.  The regions are in order of decreasing
> importance, so the most important must be applied last.
> ---
>  libavcodec/libx264.c | 50 
>  1 file changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index a3493f393d..475719021e 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -285,16 +285,18 @@ static int X264_frame(AVCodecContext *ctx,
> AVPacket *pkt, const AVFrame *frame,
>  int nnal, i, ret;
>  x264_picture_t pic_out = {0};
>  int pict_type;
> +int bit_depth;
>  int64_t *out_opaque;
>  AVFrameSideData *sd;
> 
>  x264_picture_init( &x4->pic );
>  x4->pic.img.i_csp   = x4->params.i_csp;
>  #if X264_BUILD >= 153
> -if (x4->params.i_bitdepth > 8)
> +bit_depth = x4->params.i_bitdepth;
>  #else
> -if (x264_bit_depth > 8)
> +bit_depth = x264_bit_depth;
>  #endif
> +if (bit_depth > 8)
>  x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;
>  x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
> 
> @@ -359,45 +361,47 @@ static int X264_frame(AVCodecContext *ctx,
> AVPacket *pkt, const AVFrame *frame,
>  if (frame->interlaced_frame == 0) {
>  int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
>  int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
> +int qp_range = 51 + 6 * (bit_depth - 8);

just found following from "$ ./x264 --fullhelp", not sure what 81 means here. 
Shall we change our qoffset formula accordingly?
  --qpminSet min QP [0]
  --qpmaxSet max QP [81]

>  int nb_rois;
> -AVRegionOfInterest* roi;
> -float* qoffsets;
> +const 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);
> +roi = (const AVRegionOfInterest*)sd->data;
> +if (!roi->self_size || sd->size % roi->self_size != 0) {
> +av_log(ctx, AV_LOG_ERROR, "Invalid
> AVRegionOfInterest.self_size.\n");
> +return AVERROR(EINVAL);
> +}
> +nb_rois = sd->size / roi->self_size;
> +
> +// This list must be iterated in reverse because regions 
> are
> +// defined in order of decreasing importance.

Nit:   the reason may be more straight forward.
This list must be iterated in reverse because: when overlapping regions are 
defined, the first region containing a given area of the frame applies.

> +for (int i = nb_rois - 1; i >= 0; i--) {
> +int startx, endx, starty, endy;
>  float qoffset;
> 
> +roi = (const AVRegionOfInterest*)(sd->data + 
> roi->self_size * i);
> +
> +starty = av_clip(roi->top / MB_SIZE, 0, mby);
> +endy   = av_clip((frame->height - roi->bottom + 
> MB_SIZE - 1) /
> MB_SIZE, 0, mby);
> +startx = av_clip(roi->left / MB_SIZE, 0, mbx);
> +endx   = av_clip((frame->width - roi->right + 
> MB_SIZE - 1) /
> MB_SIZE, 0, mbx);

not quite understand why endx/endy is calculated so.
For example, for a 1920x1080 frame, and roi->top is 0, and roi->bottom is 1080, 
then,
starty is 0, endy is 0, which make the following loop does not work.

  for (int y = starty; y < endy; y++) {
  for (int x = startx; x < endx; x++) {
  qoffsets[x + y*mbx] = qoffset;
  }
  }

> +
>

Re: [FFmpeg-devel] [PATCH 3/5] libx265: Update ROI behaviour to match documentation

[Guo, Yejun]

> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, February 28, 2019 6:00 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 3/5] libx265: Update ROI behaviour to
> match documentation
> 
> Equivalent to the previous patch for libx264.
> ---
>  libavcodec/libx265.c | 41 -
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
> index 98415366da..af1a2b2287 100644
> --- a/libavcodec/libx265.c
> +++ b/libavcodec/libx265.c
> @@ -296,27 +296,33 @@ static av_cold int
> libx265_encode_set_roi(libx265Context *ctx, const AVFrame *fr
>  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 qp_range = 51 + 6 * (pic->bitDepth - 8);

just found following from "$ ./x265 --fullhelp", not sure what 69 means here. 
Shall we change our qoffset formula accordingly?
   --qpmin  sets a hard lower limit on QP allowed to 
ratecontrol. Default 0
   --qpmax  sets a hard upper limit on QP allowed to 
ratecontrol. Default 69


>  int nb_rois;
> -AVRegionOfInterest *roi;
> +const 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);
> +roi = (const AVRegionOfInterest*)sd->data;
> +if (!roi->self_size || sd->size % roi->self_size != 0) {
> +av_log(ctx, AV_LOG_ERROR, "Invalid
> AVRegionOfInterest.self_size.\n");
> +return AVERROR(EINVAL);
> +}
> +nb_rois = sd->size / roi->self_size;
> +
> +// This list must be iterated in reverse because regions are
> +// defined in order of decreasing importance.

same comments as libx264

> +for (int i = nb_rois - 1; i >= 0; i--) {
> +int startx, endx, starty, endy;
>  float qoffset;
> 
> -if (roi->self_size == 0) {
> -av_free(qoffsets);
> -av_log(ctx, AV_LOG_ERROR, "AVRegionOfInterest.self_size 
> must
> be set to sizeof(AVRegionOfInterest).\n");
> -return AVERROR(EINVAL);
> -}
> +roi = (const AVRegionOfInterest*)(sd->data + roi->self_size 
> * i);
> +
> +starty = av_clip(roi->top / mb_size, 0, mby);
> +endy   = av_clip((frame->height - roi->bottom + mb_size - 1) 
> /
> mb_size, 0, mby);
> +startx = av_clip(roi->left / mb_size, 0, mbx);
> +endx   = av_clip((frame->width - roi->right + mb_size - 1) / 
> mb_size,
> 0, mbx);

same comments as libx264

> 
>  if (roi->qoffset.den == 0) {
>  av_free(qoffsets);
> @@ -324,18 +330,11 @@ static av_cold int
> libx265_encode_set_roi(libx265Context *ctx, const AVFrame *fr
>  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;
> +qoffset = av_clipf(qoffset * qp_range, -qp_range, +qp_range);
> 
>  for (int y = starty; y < endy; y++)
>  for (int x = startx; x < endx; x++)
>  qoffsets[x + y*mbx] = qoffset;
> -
> -roi = (AVRegionOfInterest*)((char*)roi + roi->self_size);
>  }
> 
>  pic->quantOffsets = qoffsets;
> --
> 2.19.2
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to match documentation

[myp...@gmail.com]

On Thu, Feb 28, 2019 at 10:53 AM Guo, Yejun  wrote:
>
>
>
> > -Original Message-
> > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> > Of Mark Thompson
> > Sent: Thursday, February 28, 2019 6:00 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to
> > match documentation
> >
> > Fix the quantisation offset - use the whole range, and don't change the
> > offset size based on bit depth.
> >
> > Use correct bottom/right edge locations (they are offsets from
bottom/right,
> > not from top/left).
> >
> > Iterate the list in reverse order.  The regions are in order of
decreasing
> > importance, so the most important must be applied last.
> > ---
> >  libavcodec/libx264.c | 50 
> >  1 file changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index a3493f393d..475719021e 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -285,16 +285,18 @@ static int X264_frame(AVCodecContext *ctx,
> > AVPacket *pkt, const AVFrame *frame,
> >  int nnal, i, ret;
> >  x264_picture_t pic_out = {0};
> >  int pict_type;
> > +int bit_depth;
> >  int64_t *out_opaque;
> >  AVFrameSideData *sd;
> >
> >  x264_picture_init( &x4->pic );
> >  x4->pic.img.i_csp   = x4->params.i_csp;
> >  #if X264_BUILD >= 153
> > -if (x4->params.i_bitdepth > 8)
> > +bit_depth = x4->params.i_bitdepth;
> >  #else
> > -if (x264_bit_depth > 8)
> > +bit_depth = x264_bit_depth;
> >  #endif
> > +if (bit_depth > 8)
> >  x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;
> >  x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
> >
> > @@ -359,45 +361,47 @@ static int X264_frame(AVCodecContext *ctx,
> > AVPacket *pkt, const AVFrame *frame,
> >  if (frame->interlaced_frame == 0) {
> >  int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
> >  int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
> > +int qp_range = 51 + 6 * (bit_depth - 8);
>
> just found following from "$ ./x264 --fullhelp", not sure what 81 means
here. Shall we change our qoffset formula accordingly?
>   --qpminSet min QP [0]
>   --qpmaxSet max QP [81]
>
> >  int nb_rois;
> > -AVRegionOfInterest* roi;
> > -float* qoffsets;
> > +const 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);
> > +roi = (const AVRegionOfInterest*)sd->data;
> > +if (!roi->self_size || sd->size % roi->self_size
!= 0) {
> > +av_log(ctx, AV_LOG_ERROR, "Invalid
> > AVRegionOfInterest.self_size.\n");
> > +return AVERROR(EINVAL);
> > +}
> > +nb_rois = sd->size / roi->self_size;
> > +
> > +// This list must be iterated in reverse because
regions are
> > +// defined in order of decreasing importance.
>
> Nit:   the reason may be more straight forward.
> This list must be iterated in reverse because: when overlapping regions
are defined, the first region containing a given area of the frame applies.
>
> > +for (int i = nb_rois - 1; i >= 0; i--) {
> > +int startx, endx, starty, endy;
> >  float qoffset;
> >
> > +roi = (const AVRegionOfInterest*)(sd->data +
roi->self_size * i);
> > +
> > +starty = av_clip(roi->top / MB_SIZE, 0, mby);
> > +endy   = av_clip((frame->height - roi->bottom
+ MB_SIZE - 1) /
> > MB_SIZE, 0, mby);
> > +startx = av_clip(roi->left / MB_SIZE, 0, mbx);
> > +endx   = av_clip((frame->width - roi->right +
MB_SIZE - 1) /
> > MB_SIZE, 0, mbx);
>
> not quite understand why endx/endy is calculated so.
> For example, for a 1920x1080 frame, and roi->top is 0, and roi->bottom is
1080, then,
> starty is 0, endy is 0, which make the following loop does not work.

I think Mark use the (left/top) and (right/bottom) as the 

[FFmpeg-devel] The log level of "co located POCs unavailable" should not be ERROR

[Yukun Guo]

The error message "co located POCs unavailable" is reported when the
co-located picture of an H.264 B frame is missing during spatial direct
predition. It comes from this line:
https://github.com/FFmpeg/FFmpeg/blob/n4.1.1/libavcodec/h264_direct.c#L156
and was originally commited for solving an issue found by fuzzing:
https://bugzilla.mozilla.org/show_bug.cgi?id=1230286.

I think the condition is quite normal for live streaming, 1) when the
player first connects to the streaming server, it may receive B frames
whose reference frames have never been sent; 2) when the bandwidth is
limited, the server purposely drops packets due to buffer overflow.
IMHO the log level should be on par with "Frame num gap" (if
`gaps_in_frame_num_value_allowed_flag` is false) and "no picture"
(both are found in h264_slice.c and use DEBUG level).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 4/5] vaapi_encode: Add ROI support

[Guo, Yejun]

> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, February 28, 2019 6:00 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 4/5] vaapi_encode: Add ROI support
> 
> ---
>  libavcodec/vaapi_encode.c   | 129
> 
>  libavcodec/vaapi_encode.h   |   9 +++
>  libavcodec/vaapi_encode_h264.c  |   2 +
>  libavcodec/vaapi_encode_h265.c  |   2 +
>  libavcodec/vaapi_encode_mpeg2.c |   2 +
>  libavcodec/vaapi_encode_vp8.c   |   2 +
>  libavcodec/vaapi_encode_vp9.c   |   2 +
>  7 files changed, 148 insertions(+)

I tried this patch with below command, but do not see any quality change with 
my eyes. 
I debugged in gdb and  found the ROI data are correct in function 
vaapi_encode_issue.

I do not investigate deeper, and just want to first confirm if you see the 
quality change or not. I might did something basic wrong.

ffmpeg_g -hwaccel vaapi -vaapi_device /dev/dri/renderD128  -s 1920x1080 -i 
../video
/1080P_park_joy_1920x1080_500frames.yuv   -vf format=nv12,hwupload-c:v 
h264_vaapi  -b:v 3M  -y tmp.264
(my trick code in vf_scale.c is called with the above command)

I tried the similar option with libx264 and found obvious video quality changes.

> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 2dda451882..1865006c1a 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -143,6 +143,7 @@ static int vaapi_encode_issue(AVCodecContext
> *avctx,
>  int err, i;
>  char data[MAX_PARAM_BUFFER_SIZE];
>  size_t bit_len;
> +AVFrameSideData *sd;
> 
>  av_log(avctx, AV_LOG_DEBUG, "Issuing encode for
> pic %"PRId64"/%"PRId64" "
> "as type %s.\n", pic->display_order, pic->encode_order,
> @@ -412,6 +413,92 @@ static int vaapi_encode_issue(AVCodecContext
> *avctx,
>  }
>  }
> 
> +sd = av_frame_get_side_data(pic->input_image,
> +AV_FRAME_DATA_REGIONS_OF_INTEREST);
> +
> +#if VA_CHECK_VERSION(1, 0, 0)
> +if (sd && ctx->roi_allowed) {
> +const AVRegionOfInterest *roi;
> +int nb_roi;
> +VAEncMiscParameterBuffer param_misc = {
> +.type = VAEncMiscParameterTypeROI,
> +};
> +VAEncMiscParameterBufferROI param_roi;
> +// VAAPI requires the second structure to immediately follow the
> +// first in the parameter buffer, but they have different natural
> +// alignment on 64-bit architectures (4 vs. 8, since the ROI
> +// structure contains a pointer).  This means we can't make a
> +// single working structure, nor can we make the buffer
> +// separately and map it because dereferencing the second pointer
> +// would be undefined.  Therefore, construct the two parts
> +// separately and combine them into a single character buffer
> +// before uploading.
> +char param_buffer[sizeof(param_misc) + sizeof(param_roi)];
> +int i, v;
> +
> +roi = (const AVRegionOfInterest*)sd->data;
> +av_assert0(roi->self_size && sd->size % roi->self_size == 0);
> +nb_roi = sd->size / roi->self_size;
> +if (nb_roi > ctx->roi_regions) {
> +if (!ctx->roi_warned) {
> +av_log(avctx, AV_LOG_WARNING, "More ROIs set than "
> +   "supported by driver (%d > %d).\n",
> +   nb_roi, ctx->roi_regions);
> +ctx->roi_warned = 1;
> +}
> +nb_roi = ctx->roi_regions;
> +}
> +
> +pic->roi = av_mallocz_array(nb_roi, sizeof(*pic->roi));
> +if (!pic->roi) {
> +err = AVERROR(ENOMEM);
> +goto fail;
> +}
> +for (i = 0; i < nb_roi; i++) {
> +roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
> +
> +v = roi->qoffset.num * ctx->roi_quant_range / roi->qoffset.den;
> +av_log(avctx, AV_LOG_DEBUG, "ROI region: (%d-%d,%d-%d) -
> > %+d.\n",
> +   roi->left, avctx->width - roi->right,
> +   roi->top, avctx->height - roi->bottom, v);
> +
> +pic->roi[i] = (VAEncROI) {
> +.roi_rectangle = {
> +.x  = roi->left,
> +.y  = roi->top,
> +.width  = avctx->width  - roi->right - roi->left,
> +.height = avctx->height - roi->bottom - roi->top,
> +},
> +.roi_value = av_clip_c(v, INT8_MIN, INT8_MAX),
> +};
> +}
> +
> +param_roi = (VAEncMiscParameterBufferROI) {
> +.num_roi  = nb_roi,
> +.max_delta_qp = INT8_MAX,
> +.min_delta_qp = 0,
> +.roi  = pic->roi,
> +.roi_flags.bits.roi_value_is_qp_delta = 1,
> +};
> +
> +memcpy(param_buffer, ¶m_misc, sizeof(param_misc));

Re: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to match documentation

[Guo, Yejun]

> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of myp...@gmail.com
> Sent: Thursday, February 28, 2019 11:26 AM
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to
> match documentation
> 
> On Thu, Feb 28, 2019 at 10:53 AM Guo, Yejun  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On
> Behalf
> > > Of Mark Thompson
> > > Sent: Thursday, February 28, 2019 6:00 AM
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: [FFmpeg-devel] [PATCH 2/5] libx264: Update ROI behaviour to
> > > match documentation
> > >
> > > Fix the quantisation offset - use the whole range, and don't change the
> > > offset size based on bit depth.
> > >
> > > Use correct bottom/right edge locations (they are offsets from
> bottom/right,
> > > not from top/left).
> > >
> > > Iterate the list in reverse order.  The regions are in order of
> decreasing
> > > importance, so the most important must be applied last.
> > > ---
> > >  libavcodec/libx264.c | 50 
> > >  1 file changed, 27 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > > index a3493f393d..475719021e 100644
> > > --- a/libavcodec/libx264.c
> > > +++ b/libavcodec/libx264.c
> > > @@ -285,16 +285,18 @@ static int X264_frame(AVCodecContext *ctx,
> > > AVPacket *pkt, const AVFrame *frame,
> > >  int nnal, i, ret;
> > >  x264_picture_t pic_out = {0};
> > >  int pict_type;
> > > +int bit_depth;
> > >  int64_t *out_opaque;
> > >  AVFrameSideData *sd;
> > >
> > >  x264_picture_init( &x4->pic );
> > >  x4->pic.img.i_csp   = x4->params.i_csp;
> > >  #if X264_BUILD >= 153
> > > -if (x4->params.i_bitdepth > 8)
> > > +bit_depth = x4->params.i_bitdepth;
> > >  #else
> > > -if (x264_bit_depth > 8)
> > > +bit_depth = x264_bit_depth;
> > >  #endif
> > > +if (bit_depth > 8)
> > >  x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;
> > >  x4->pic.img.i_plane = avfmt2_num_planes(ctx->pix_fmt);
> > >
> > > @@ -359,45 +361,47 @@ static int X264_frame(AVCodecContext *ctx,
> > > AVPacket *pkt, const AVFrame *frame,
> > >  if (frame->interlaced_frame == 0) {
> > >  int mbx = (frame->width + MB_SIZE - 1) / MB_SIZE;
> > >  int mby = (frame->height + MB_SIZE - 1) / MB_SIZE;
> > > +int qp_range = 51 + 6 * (bit_depth - 8);
> >
> > just found following from "$ ./x264 --fullhelp", not sure what 81 means
> here. Shall we change our qoffset formula accordingly?
> >   --qpminSet min QP [0]
> >   --qpmaxSet max QP [81]
> >
> > >  int nb_rois;
> > > -AVRegionOfInterest* roi;
> > > -float* qoffsets;
> > > +const 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);
> > > +roi = (const AVRegionOfInterest*)sd->data;
> > > +if (!roi->self_size || sd->size % roi->self_size
> != 0) {
> > > +av_log(ctx, AV_LOG_ERROR, "Invalid
> > > AVRegionOfInterest.self_size.\n");
> > > +return AVERROR(EINVAL);
> > > +}
> > > +nb_rois = sd->size / roi->self_size;
> > > +
> > > +// This list must be iterated in reverse because
> regions are
> > > +// defined in order of decreasing importance.
> >
> > Nit:   the reason may be more straight forward.
> > This list must be iterated in reverse because: when overlapping regions
> are defined, the first region containing a given area of the frame applies.
> >
> > > +for (int i = nb_rois - 1; i >= 0; i--) {
> > > +int startx, endx, starty, endy;
> > >  float qoffset;
> > >
> > > +roi = (const AVRegionOfInterest*)(sd->data +
> roi->self_size * i);
> > > +
> > > +starty = av_clip(roi->top / MB_SIZE, 0, mby);
> > > +