[FFmpeg-devel] [PATCH v2 1/6] lavu/frame: Change region of interest rectangle definition

[Mark Thompson]

The definition here previously matched the way cropping worked, but the
actual uses in libavcodec instead treated the values as the coordinates
of the top-left and bottom-right corners.

The most common way of defining rectangles in FFmpeg appears to be as
coordinates of the top-left corner combined with a width and height; so,
change to using that instead.  This is technically both an API and an ABI
break, but since the structure was added recently and the previous
definition was never actually used correctly I believe that this should
not cause any problems.
---
This seems to be by far the most common choice of rectangle definition, so I think the most consistent one to use.

If you think the break is a step too far (if there are active users then maybe it's left a bit too long since the API was added) then please do say and I'll revert back to the version keeping the currently-documented cropping behaviour.

(Actual version bump to add.)

- Mark


 doc/APIchanges       |  3 +++
 libavcodec/libx264.c |  8 ++++----
 libavcodec/libx265.c |  8 ++++----
 libavutil/frame.h    | 20 ++++++++++++--------
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 784a5e5bd2..4e3b5d6b5a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@ libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2019-03-XX - XXXXXXXXXX - lavc 56.XX.100 - frame.h
+  Change definition of AVRegionOfInterest.
+
 2019-01-27 - XXXXXXXXXX - lavc 58.46.100 - avcodec.h
   Add discard_damaged_percentage
 
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index a3493f393d..87e6fe7c94 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -369,10 +369,10 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
                     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);
+                        int starty = av_clip(roi->y / MB_SIZE, 0, mby);
+                        int endy   = av_clip((roi->y + roi->height + MB_SIZE - 1) / MB_SIZE, 0, mby);
+                        int startx = av_clip(roi->x / MB_SIZE, 0, mbx);
+                        int endx   = av_clip((roi->x + roi->width  + MB_SIZE - 1) / MB_SIZE, 0, mbx);
                         float qoffset;
 
                         if (roi->qoffset.den == 0) {
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index fe39f45241..c2c1f8b9bc 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -314,10 +314,10 @@ static av_cold int libx265_encode_set_roi(libx265Context *ctx, const AVFrame *fr
             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);
+                int starty = av_clip(roi->y / mb_size, 0, mby);
+                int endy   = av_clip((roi->y + roi->height + mb_size - 1) / mb_size, 0, mby);
+                int startx = av_clip(roi->x / mb_size, 0, mbx);
+                int endx   = av_clip((roi->x + roi->width  + mb_size - 1) / mb_size, 0, mbx);
                 float qoffset;
 
                 if (roi->self_size == 0) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 8aa3e88367..cc3b78d8b6 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -212,10 +212,6 @@ typedef struct AVFrameSideData {
  * 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.
  *
- * 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:
@@ -228,10 +224,18 @@ typedef struct AVFrameSideData {
  */
 typedef struct AVRegionOfInterest {
     uint32_t self_size;
-    int top;
-    int bottom;
-    int left;
-    int right;
+    /**
+     * x/y coordinates of the top-left corner and width/height of the
+     * rectangle defining this 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 x;
+    int y;
+    int width;
+    int height;
     AVRational qoffset;
 } AVRegionOfInterest;
 
-- 
2.19.2