[FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

This includes a struct and helpers. It will be used to support container level
cropping and tiled image formats, but should be generic enough for general
usage.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Extended to include fields used for cropping. Should make the struct reusable
even for non tiled images, e.g. setting both rows and tiles to 1, in which case
tile width and height would become analogous to coded_{witdh,height}.

 libavutil/Makefile |   2 +
 libavutil/tile.c   |  81 +++++++++++++++++++
 libavutil/tile.h   | 188 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 libavutil/tile.c
 create mode 100644 libavutil/tile.h

diff --git a/libavutil/Makefile b/libavutil/Makefile
index e7709b97d0..380d706cfe 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -82,6 +82,7 @@ HEADERS = adler32.h                                                     \
           spherical.h                                                   \
           stereo3d.h                                                    \
           threadmessage.h                                               \
+          tile.h                                                        \
           time.h                                                        \
           timecode.h                                                    \
           timestamp.h                                                   \
@@ -172,6 +173,7 @@ OBJS = adler32.o                                                        \
        spherical.o                                                      \
        stereo3d.o                                                       \
        threadmessage.o                                                  \
+       tile.o                                                           \
        time.o                                                           \
        timecode.o                                                       \
        tree.o                                                           \
diff --git a/libavutil/tile.c b/libavutil/tile.c
new file mode 100644
index 0000000000..de59ba4bab
--- /dev/null
+++ b/libavutil/tile.c
@@ -0,0 +1,81 @@
+/*
+ * 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 <stdint.h>
+#include <limits.h>
+
+#include "log.h"
+#include "mem.h"
+#include "opt.h"
+#include "tile.h"
+
+#define FLAGS AV_OPT_FLAG_ENCODING_PARAM | AV_OPT_FLAG_VIDEO_PARAM
+#define OFFSET(x) offsetof(AVTileGrid, x)
+static const AVOption tile_grid_options[] = {
+    { "type", NULL, OFFSET(type), AV_OPT_TYPE_INT,
+            { .i64 = AV_TILE_DIMENSION_TYPE_UNIFORM },
+            AV_TILE_DIMENSION_TYPE_UNIFORM, AV_TILE_DIMENSION_TYPE_VARIABLE, FLAGS, "type" },
+        { "uniform",  NULL, 0, AV_OPT_TYPE_CONST,
+                   { .i64 = AV_TILE_DIMENSION_TYPE_UNIFORM },  .unit = "type" },
+        { "variable", NULL, 0, AV_OPT_TYPE_CONST,
+                   { .i64 = AV_TILE_DIMENSION_TYPE_VARIABLE }, .unit = "type" },
+    { "tile_rows", NULL, OFFSET(tile_rows), AV_OPT_TYPE_INT, { .i64 = 1 }, 1, INT_MAX, FLAGS },
+    { "tile_cols", NULL, OFFSET(tile_cols), AV_OPT_TYPE_INT, { .i64 = 1 }, 1, INT_MAX, FLAGS },
+    { "horizontal_offset", NULL, OFFSET(horizontal_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, FLAGS },
+    { "vertical_offset", NULL, OFFSET(vertical_offset),     AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, FLAGS },
+    { "output_size", "size of the output image", OFFSET(output_width), AV_OPT_TYPE_IMAGE_SIZE,
+        { .str = NULL }, 0, INT_MAX, FLAGS },
+    { NULL },
+};
+
+static const AVClass tile_grid_class = {
+    .class_name          = "AVTileGrid",
+    .version             = LIBAVUTIL_VERSION_INT,
+    .option              = tile_grid_options,
+};
+
+const AVClass *av_tile_grid_get_class(void)
+{
+    return &tile_grid_class;
+}
+
+AVTileGrid *av_tile_grid_alloc(void)
+{
+    AVTileGrid *tile_grid = av_mallocz(sizeof(AVTileGrid));
+
+    if (tile_grid) {
+        tile_grid->av_class = &tile_grid_class;
+        av_opt_set_defaults(tile_grid);
+    }
+
+    return tile_grid;
+}
+
+void av_tile_grid_free(AVTileGrid **ptile_grid)
+{
+    AVTileGrid *tile_grid = *ptile_grid;
+
+    if (!tile_grid)
+        return;
+
+    if (tile_grid->type == AV_TILE_DIMENSION_TYPE_VARIABLE) {
+        av_freep(&tile_grid->w.tile_widths);
+        av_freep(&tile_grid->h.tile_heights);
+    }
+    av_freep(ptile_grid);
+}
diff --git a/libavutil/tile.h b/libavutil/tile.h
new file mode 100644
index 0000000000..7e68dc7bd5
--- /dev/null
+++ b/libavutil/tile.h
@@ -0,0 +1,188 @@
+/*
+ * 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
+ */
+
+#ifndef AVUTIL_TILE_H
+#define AVUTIL_TILE_H
+
+#include <stdint.h>
+
+#include "log.h"
+
+/**
+ * @defgroup lavu_tile_grid Tile grid paremeters
+ * @{
+ */
+
+enum AVTileGridType {
+    AV_TILE_DIMENSION_TYPE_UNIFORM,  ///< All tiles have the same size
+    AV_TILE_DIMENSION_TYPE_VARIABLE, ///< Tiles may have variable size
+};
+
+/**
+ * AVTileGrid holds information on how to combine several independent images in a
+ * single grid for presentation.
+ *
+ * It must be allocated by av_tile_grid_alloc(), and its size is not a part of the
+ * ABI. No new fields may be added to this struct without a major version bump,
+ * except for new elements of the unions w and h fitting in sizeof(intptr_t).
+ */
+typedef struct AVTileGrid {
+    const AVClass *av_class;
+
+    /**
+     * Amount of rows in the grid.
+     *
+     * Must be > 0.
+     */
+    int tile_rows;
+    /**
+     * Amount of columns in the grid.
+     *
+     * Must be > 0.
+     */
+    int tile_cols;
+
+    enum AVTileGridType type;
+
+    /**
+     * Tile witdh paremeters
+     */
+    union {
+        /**
+         * Width of every tile.
+         * This member must be used when the @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM.
+         */
+        intptr_t tile_width;
+        /**
+         * A @ref tile_rows * @ref tile_cols sized array of width values for each
+         * tile in the grid, in row major order.
+         * This member must be used when the @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE.
+         *
+         * Must be allocated with the av_malloc() family of functions, and will be
+         * freed by av_tile_grid_free().
+         */
+        int *tile_widths;
+    } w;
+
+    /**
+     * Tile height paremeters
+     */
+    union {
+        /**
+         * Height of every tile.
+         *
+         * This member must be used when the @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM.
+         */
+        intptr_t tile_height;
+        /**
+         * A @ref tile_rows * @ref tile_cols sized array of height values for each
+         * tile in the grid, in row major order.
+         *
+         * This member must be used when the @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE.
+         * Must be allocated with the av_malloc() family of functions, and will be
+         * freed by av_tile_grid_free().
+         */
+        int *tile_heights;
+    } h;
+
+    /**
+     * Offset in pixels from the left edge of the grid where the actual image
+     * meant for presentation starts.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM, this field must be >= 0
+     * and <= ((@ref tile_width * @ref tile_cols) - @ref output_width).
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE, this field must be >= 0
+     * and <= the sum of all values in the tile_widths array minus
+     * @ref output_width.
+     */
+    int horizontal_offset;
+    /**
+     * Offset in pixels from the top edge of the grid where the actual image meant
+     * for presentation starts.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM, this field must be >= 0
+     * and <= ((@ref tile_height * @ref tile_rows) - @ref output_height).
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE, this field must be >= 0
+     * and <= the sum of all values in the tile_heights array minus
+     * @ref output_height.
+     */
+    int vertical_offset;
+
+    /**
+     * Width of the final image for presentation.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM, this field must be > 0
+     * and <= (@ref tile_width * @ref tile_cols) - @ref horizontal_offset.
+     * When it's not equal to tile_width * tile_cols, the result of
+     * (@ref tile_width * @ref tile_cols) - output_width - @ref horizontal_offset
+     * is the amount of pixels to be cropped from the right edge of the final
+     * image before presentation.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE, this field must be > 0
+     * and <= the sum of all values in the tile_widths array minus
+     * @ref horizontal_offset.
+     * When it's not equal to the sum of all values in the tile_widths array,
+     * the result of said sum minus output_width minus @ref horizontal_offset is
+     * the amount of pixels to be cropped from the right edge of the final image
+     * before presentation.
+     */
+    int output_width;
+    /**
+     * Height of the final image for presentation.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_UNIFORM, this field must be > 0
+     * and <= (@ref tile_height * @ref tile_rows) - @ref vertical_offset.
+     * When it's not equal to tile_height * tile_rows, the result of
+     * (@ref tile_height * @ref tile_rows) - output_height - @ref vertical_offset
+     * is the amount of pixels to be cropped from the bottom edge of the final
+     * image before presentation.
+     *
+     * When @ref type is AV_TILE_DIMENSION_TYPE_VARIABLE, this field must be > 0
+     * and <= the sum of all values in the tile_heights array minus
+     * @ref vertical_offset.
+     * When it's not equal to the sum of all values in the tile_heights array,
+     * the result of said sum minus output_height minus @ref vertical_offset is
+     * the amount of pixels to be cropped from the bottom edge of the final image
+     * before presentation.
+     */
+    int output_height;
+} AVTileGrid;
+
+const AVClass *av_tile_grid_get_class(void);
+
+/**
+ * Allocates a AVTileGrid, and initializes its fields with default values.
+ * Must be freed with av_tile_grid_free().
+ */
+AVTileGrid *av_tile_grid_alloc(void);
+
+/**
+ * Free an AVTileGrid and all its contents.
+ *
+ * @param tile_grid pointer to pointer to an allocated AVTileGrid.
+ *                  upon return, *tile_grid will be set to NULL.
+ */
+void av_tile_grid_free(AVTileGrid **tile_grid);
+
+/**
+ * @}
+ */
+
+#endif /* AVUTIL_TILE_H */
-- 
2.43.0

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

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2 v2] avformat: add a Tile Grid stream group type

[James Almer]

This will be used to support tiled image formats like HEIF.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/avformat.c |  5 +++++
 libavformat/avformat.h |  3 +++
 libavformat/dump.c     | 46 ++++++++++++++++++++++++++++++++++++++++++
 libavformat/options.c  |  9 +++++++++
 4 files changed, 63 insertions(+)

diff --git a/libavformat/avformat.c b/libavformat/avformat.c
index 882927f7b1..a7bd959db5 100644
--- a/libavformat/avformat.c
+++ b/libavformat/avformat.c
@@ -30,6 +30,7 @@
 #include "libavutil/opt.h"
 #include "libavutil/pixfmt.h"
 #include "libavutil/samplefmt.h"
+#include "libavutil/tile.h"
 #include "libavcodec/avcodec.h"
 #include "libavcodec/codec.h"
 #include "libavcodec/bsf.h"
@@ -100,6 +101,10 @@ void ff_free_stream_group(AVStreamGroup **pstg)
         av_iamf_mix_presentation_free(&stg->params.iamf_mix_presentation);
         break;
     }
+    case AV_STREAM_GROUP_PARAMS_TILE_GRID: {
+        av_tile_grid_free(&stg->params.tile_grid);
+        break;
+    }
     default:
         break;
     }
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 5d0fe82250..f259ad1367 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1022,10 +1022,12 @@ enum AVStreamGroupParamsType {
     AV_STREAM_GROUP_PARAMS_NONE,
     AV_STREAM_GROUP_PARAMS_IAMF_AUDIO_ELEMENT,
     AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION,
+    AV_STREAM_GROUP_PARAMS_TILE_GRID,
 };
 
 struct AVIAMFAudioElement;
 struct AVIAMFMixPresentation;
+struct AVTileGrid;
 
 typedef struct AVStreamGroup {
     /**
@@ -1062,6 +1064,7 @@ typedef struct AVStreamGroup {
     union {
         struct AVIAMFAudioElement *iamf_audio_element;
         struct AVIAMFMixPresentation *iamf_mix_presentation;
+        struct AVTileGrid *tile_grid;
     } params;
 
     /**
diff --git a/libavformat/dump.c b/libavformat/dump.c
index aff51b43f6..2927c790a8 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <stdint.h>
 
+#include "libavutil/avstring.h"
 #include "libavutil/channel_layout.h"
 #include "libavutil/display.h"
 #include "libavutil/iamf.h"
@@ -35,6 +36,7 @@
 #include "libavutil/spherical.h"
 #include "libavutil/stereo3d.h"
 #include "libavutil/timecode.h"
+#include "libavutil/tile.h"
 
 #include "libavcodec/avcodec.h"
 
@@ -720,6 +722,50 @@ static void dump_stream_group(const AVFormatContext *ic, uint8_t *printed,
             }
         }
         break;
+    case AV_STREAM_GROUP_PARAMS_TILE_GRID: {
+        const AVTileGrid *tile_grid = stg->params.tile_grid;
+        AVCodecContext *avctx = avcodec_alloc_context3(NULL);
+        const char *ptr = NULL;
+        av_log(NULL, AV_LOG_INFO, " Tile Grid:");
+        av_log(NULL, AV_LOG_VERBOSE, " %d rows, %d columns,", tile_grid->tile_rows, tile_grid->tile_cols);
+        if (avctx && stg->nb_streams && !avcodec_parameters_to_context(avctx, stg->streams[0]->codecpar)) {
+            int coded_width = 0, coded_height = 0;
+            avctx->width  = tile_grid->output_width;
+            avctx->height = tile_grid->output_height;
+            switch (tile_grid->type) {
+            case AV_TILE_DIMENSION_TYPE_UNIFORM:
+                coded_width  = FFALIGN(tile_grid->output_width,  tile_grid->w.tile_width);
+                coded_height = FFALIGN(tile_grid->output_height, tile_grid->h.tile_height);
+                break;
+            case AV_TILE_DIMENSION_TYPE_VARIABLE: {
+                int size = tile_grid->tile_rows * tile_grid->tile_cols;
+                for (int j = 0; j < tile_grid->tile_cols; j++)
+                    coded_width += tile_grid->w.tile_widths[j];
+                for (int j = 0; j < size; j += tile_grid->tile_cols)
+                    coded_height += tile_grid->h.tile_heights[j];
+                break;
+            }
+            }
+            avctx->coded_width  = coded_width;
+            avctx->coded_height = coded_height;
+            if (ic->dump_separator)
+                av_opt_set(avctx, "dump_separator", ic->dump_separator, 0);
+            buf[0] = 0;
+            avcodec_string(buf, sizeof(buf), avctx, is_output);
+            ptr = av_stristr(buf, " ");
+        }
+        avcodec_free_context(&avctx);
+        if (ptr)
+            av_log(NULL, AV_LOG_INFO, "%s", ptr);
+        av_log(NULL, AV_LOG_INFO, "\n");
+        dump_metadata(NULL, stg->metadata, "    ", AV_LOG_INFO);
+        for (int i = 0; i < stg->nb_streams; i++) {
+            const AVStream *st = stg->streams[i];
+            dump_stream_format(ic, st->index, i, index, is_output, AV_LOG_VERBOSE);
+            printed[st->index] = 1;
+        }
+        break;
+    }
     }
     default:
         break;
diff --git a/libavformat/options.c b/libavformat/options.c
index 75ec86ce05..b6b7eddd40 100644
--- a/libavformat/options.c
+++ b/libavformat/options.c
@@ -30,6 +30,7 @@
 #include "libavutil/internal.h"
 #include "libavutil/intmath.h"
 #include "libavutil/opt.h"
+#include "libavutil/tile.h"
 
 /**
  * @file
@@ -368,6 +369,9 @@ static const AVClass *stream_group_child_iterate(void **opaque)
     case AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION:
         ret = av_iamf_mix_presentation_get_class();
         break;
+    case AV_STREAM_GROUP_PARAMS_TILE_GRID:
+        ret = av_tile_grid_get_class();
+        break;
     default:
         break;
     }
@@ -429,6 +433,11 @@ AVStreamGroup *avformat_stream_group_create(AVFormatContext *s,
         if (!stg->params.iamf_mix_presentation)
             goto fail;
         break;
+    case AV_STREAM_GROUP_PARAMS_TILE_GRID:
+        stg->params.tile_grid = av_tile_grid_alloc();
+        if (!stg->params.tile_grid)
+            goto fail;
+        break;
     default:
         goto fail;
     }
-- 
2.43.0

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

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-20 23:04:06)
> This includes a struct and helpers. It will be used to support container level
> cropping and tiled image formats, but should be generic enough for general
> usage.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Extended to include fields used for cropping. Should make the struct reusable
> even for non tiled images, e.g. setting both rows and tiles to 1, in which case
> tile width and height would become analogous to coded_{witdh,height}.

But why? What does cropping have to do with tiling? What advantage is
there to handling them in one struct?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/21/2024 3:27 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-20 23:04:06)
>> This includes a struct and helpers. It will be used to support container level
>> cropping and tiled image formats, but should be generic enough for general
>> usage.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Extended to include fields used for cropping. Should make the struct reusable
>> even for non tiled images, e.g. setting both rows and tiles to 1, in which case
>> tile width and height would become analogous to coded_{witdh,height}.
> 
> But why? What does cropping have to do with tiling? What advantage is
> there to handling them in one struct?

The struct does not need to be used for non tiled image scenarios, but 
could if we decide we don't want to add another struct that would only 
contain a subset of the fields present here.

As to why said fields here present here, HEIF may use a clap box to 
define cropping for the final image, not for the tiles. This needs to be 
propagated, and the previous version of this API, which only defined 
cropping from right and bottom edges if output dimensions were smaller 
than the grid (standard case for tiled heif with no clap box), was not 
enough. Hence this change.

I can rename this struct to Image Grid or something else, which might 
make it feel less awkward if we decide to reuse it. We still need to 
propagate container cropping from clap boxes and from Matroska elements 
after all.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-21 13:06:28)
> On 1/21/2024 3:27 AM, Anton Khirnov wrote:
> > Quoting James Almer (2024-01-20 23:04:06)
> >> This includes a struct and helpers. It will be used to support container level
> >> cropping and tiled image formats, but should be generic enough for general
> >> usage.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> Extended to include fields used for cropping. Should make the struct reusable
> >> even for non tiled images, e.g. setting both rows and tiles to 1, in which case
> >> tile width and height would become analogous to coded_{witdh,height}.
> > 
> > But why? What does cropping have to do with tiling? What advantage is
> > there to handling them in one struct?
> 
> The struct does not need to be used for non tiled image scenarios, but 
> could if we decide we don't want to add another struct that would only 
> contain a subset of the fields present here.
> 
> As to why said fields here present here, HEIF may use a clap box to 
> define cropping for the final image, not for the tiles. This needs to be 
> propagated, and the previous version of this API, which only defined 
> cropping from right and bottom edges if output dimensions were smaller 
> than the grid (standard case for tiled heif with no clap box), was not 
> enough. Hence this change.
> 
> I can rename this struct to Image Grid or something else, which might 
> make it feel less awkward if we decide to reuse it. We still need to 
> propagate container cropping from clap boxes and from Matroska elements 
> after all.

Honestly this whole new API strikes me as massively overthinking it. All
you should need to describe an arbitrary partition of an image into
sub-rectangles is an array of (x, y, width, height). Instead you're
proposing a new public header, struct, three functions, multiple "tile
types", and if I'm not mistaken it still cannot describe an arbitrary
partitioning. Plus it's in libavutil for some reason, even though
libavformat seems to be the only intended user.

Is all this complexity really warranted?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/21/2024 2:29 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 13:06:28)
>> On 1/21/2024 3:27 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2024-01-20 23:04:06)
>>>> This includes a struct and helpers. It will be used to support container level
>>>> cropping and tiled image formats, but should be generic enough for general
>>>> usage.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> Extended to include fields used for cropping. Should make the struct reusable
>>>> even for non tiled images, e.g. setting both rows and tiles to 1, in which case
>>>> tile width and height would become analogous to coded_{witdh,height}.
>>>
>>> But why? What does cropping have to do with tiling? What advantage is
>>> there to handling them in one struct?
>>
>> The struct does not need to be used for non tiled image scenarios, but
>> could if we decide we don't want to add another struct that would only
>> contain a subset of the fields present here.
>>
>> As to why said fields here present here, HEIF may use a clap box to
>> define cropping for the final image, not for the tiles. This needs to be
>> propagated, and the previous version of this API, which only defined
>> cropping from right and bottom edges if output dimensions were smaller
>> than the grid (standard case for tiled heif with no clap box), was not
>> enough. Hence this change.
>>
>> I can rename this struct to Image Grid or something else, which might
>> make it feel less awkward if we decide to reuse it. We still need to
>> propagate container cropping from clap boxes and from Matroska elements
>> after all.
> 
> Honestly this whole new API strikes me as massively overthinking it. All
> you should need to describe an arbitrary partition of an image into
> sub-rectangles is an array of (x, y, width, height). Instead you're
> proposing a new public header, struct, three functions, multiple "tile
> types", and if I'm not mistaken it still cannot describe an arbitrary
> partitioning. Plus it's in libavutil for some reason, even though
> libavformat seems to be the only intended user.
> 
> Is all this complexity really warranted?

1. It needs to be usable as a Stream Group type, so a struct is 
required. Said struct needs an allocator unless we want to have its size 
be part of the ABI. I can remove the free function, but then the caller 
needs to manually free any internal data.
2. We need tile dimensions (Width and height) plus row and column count, 
which give you the final size of the grid, then offsets x and y to get 
the actual image within the grid meant for presentation.
3. I want to support uniform tiles as well as variable tile dimensions, 
hence multiple tile types. The latter currently has no use case, but 
eventually might. I can if you prefer not include said type at first, 
but i want to keep the union in place so it and other extensions can be 
added.
4. It's in lavu because its meant to be generic. It can also be used to 
transport tiling and cropping information as stream and packet side 
data, which can't depend on something defined in lavf.

And what do you mean with not supporting describing arbitrary 
partitioning? Isn't that what variable tile dimensions achieve?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-21 18:47:43)
> On 1/21/2024 2:29 PM, Anton Khirnov wrote:
> > Honestly this whole new API strikes me as massively overthinking it. All
> > you should need to describe an arbitrary partition of an image into
> > sub-rectangles is an array of (x, y, width, height). Instead you're
> > proposing a new public header, struct, three functions, multiple "tile
> > types", and if I'm not mistaken it still cannot describe an arbitrary
> > partitioning. Plus it's in libavutil for some reason, even though
> > libavformat seems to be the only intended user.
> > 
> > Is all this complexity really warranted?
> 
> 1. It needs to be usable as a Stream Group type, so a struct is 
> required. Said struct needs an allocator unless we want to have its size 
> be part of the ABI. I can remove the free function, but then the caller 
> needs to manually free any internal data.

If the struct lives in lavf and is always allocated as a part of
AVStreamGroup then you don't need a public constructor/destructor and
can still extend the struct.

> 2. We need tile dimensions (Width and height) plus row and column count, 
> which give you the final size of the grid, then offsets x and y to get 
> the actual image within the grid meant for presentation.
> 3. I want to support uniform tiles as well as variable tile dimensions, 
> hence multiple tile types. The latter currently has no use case, but 
> eventually might. I can if you prefer not include said type at first, 
> but i want to keep the union in place so it and other extensions can be 
> added.
> 4. It's in lavu because its meant to be generic. It can also be used to 
> transport tiling and cropping information as stream and packet side 
> data, which can't depend on something defined in lavf.

When would you have tiling information associated with a specific
stream?

> And what do you mean with not supporting describing arbitrary 
> partitioning? Isn't that what variable tile dimensions achieve?

IIUC your tiling scheme still assumes that the partitioning is by rows
and columns. A completely generic partitioning could be irregular.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/21/2024 3:29 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 18:47:43)
>> On 1/21/2024 2:29 PM, Anton Khirnov wrote:
>>> Honestly this whole new API strikes me as massively overthinking it. All
>>> you should need to describe an arbitrary partition of an image into
>>> sub-rectangles is an array of (x, y, width, height). Instead you're
>>> proposing a new public header, struct, three functions, multiple "tile
>>> types", and if I'm not mistaken it still cannot describe an arbitrary
>>> partitioning. Plus it's in libavutil for some reason, even though
>>> libavformat seems to be the only intended user.
>>>
>>> Is all this complexity really warranted?
>>
>> 1. It needs to be usable as a Stream Group type, so a struct is
>> required. Said struct needs an allocator unless we want to have its size
>> be part of the ABI. I can remove the free function, but then the caller
>> needs to manually free any internal data.
> 
> If the struct lives in lavf and is always allocated as a part of
> AVStreamGroup then you don't need a public constructor/destructor and
> can still extend the struct.

Yes, but that would be the case if it's only meant to be allocated by 
AVStreamGroup and nothing else.

> 
>> 2. We need tile dimensions (Width and height) plus row and column count,
>> which give you the final size of the grid, then offsets x and y to get
>> the actual image within the grid meant for presentation.
>> 3. I want to support uniform tiles as well as variable tile dimensions,
>> hence multiple tile types. The latter currently has no use case, but
>> eventually might. I can if you prefer not include said type at first,
>> but i want to keep the union in place so it and other extensions can be
>> added.
>> 4. It's in lavu because its meant to be generic. It can also be used to
>> transport tiling and cropping information as stream and packet side
>> data, which can't depend on something defined in lavf.
> 
> When would you have tiling information associated with a specific
> stream?

Can't think of an example for tiling, but i can for cropping. If you 
insist on not reusing this for non-HEIF cropping usage in mp4/matroska, 
then ok, I'll move it to lavf.

> 
>> And what do you mean with not supporting describing arbitrary
>> partitioning? Isn't that what variable tile dimensions achieve?
> 
> IIUC your tiling scheme still assumes that the partitioning is by rows
> and columns. A completely generic partitioning could be irregular.

A new tile type that doesn't define rows and columns can be added if 
needed. But the current variable tile type can support things like grids 
of two rows and two columns where the second row is effectively a single 
tile, simply by setting the second tile in said row as having a width of 0.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-21 19:38:50)
> On 1/21/2024 3:29 PM, Anton Khirnov wrote:
> > Quoting James Almer (2024-01-21 18:47:43)
> >> On 1/21/2024 2:29 PM, Anton Khirnov wrote:
> >>> Honestly this whole new API strikes me as massively overthinking it. All
> >>> you should need to describe an arbitrary partition of an image into
> >>> sub-rectangles is an array of (x, y, width, height). Instead you're
> >>> proposing a new public header, struct, three functions, multiple "tile
> >>> types", and if I'm not mistaken it still cannot describe an arbitrary
> >>> partitioning. Plus it's in libavutil for some reason, even though
> >>> libavformat seems to be the only intended user.
> >>>
> >>> Is all this complexity really warranted?
> >>
> >> 1. It needs to be usable as a Stream Group type, so a struct is
> >> required. Said struct needs an allocator unless we want to have its size
> >> be part of the ABI. I can remove the free function, but then the caller
> >> needs to manually free any internal data.
> > 
> > If the struct lives in lavf and is always allocated as a part of
> > AVStreamGroup then you don't need a public constructor/destructor and
> > can still extend the struct.
> 
> Yes, but that would be the case if it's only meant to be allocated by 
> AVStreamGroup and nothing else.

That is the case right now, no?

If that ever changes then the constructor can be added.

> > 
> >> 2. We need tile dimensions (Width and height) plus row and column count,
> >> which give you the final size of the grid, then offsets x and y to get
> >> the actual image within the grid meant for presentation.
> >> 3. I want to support uniform tiles as well as variable tile dimensions,
> >> hence multiple tile types. The latter currently has no use case, but
> >> eventually might. I can if you prefer not include said type at first,
> >> but i want to keep the union in place so it and other extensions can be
> >> added.
> >> 4. It's in lavu because its meant to be generic. It can also be used to
> >> transport tiling and cropping information as stream and packet side
> >> data, which can't depend on something defined in lavf.
> > 
> > When would you have tiling information associated with a specific
> > stream?
> 
> Can't think of an example for tiling, but i can for cropping. If you 
> insist on not reusing this for non-HEIF cropping usage in mp4/matroska, 
> then ok, I'll move it to lavf.

I still don't see why should it be a good idea to use this struct for
generic container cropping. It feels very much like a hammer in search
of a nail.

> > 
> >> And what do you mean with not supporting describing arbitrary
> >> partitioning? Isn't that what variable tile dimensions achieve?
> > 
> > IIUC your tiling scheme still assumes that the partitioning is by rows
> > and columns. A completely generic partitioning could be irregular.
> 
> A new tile type that doesn't define rows and columns can be added if 
> needed. But the current variable tile type can support things like grids 
> of two rows and two columns where the second row is effectively a single 
> tile, simply by setting the second tile in said row as having a width of 0.

The problem I see here is that every consumer of this struct then has to
explicitly support every type, and adding a new type requires updating
all callers. This seems unnecessary when "list of N rectangles" covers
all possible partitionings.

That does not mean you actually have to store it that way - the struct
could be a list of N rectangles logically, while actually being
represented more efficiently (in the same way a channel layout is always
logically a list of channels, even though it's often represented by an
uint64 rather than a malloced array).

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/21/2024 4:02 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 19:38:50)
>> On 1/21/2024 3:29 PM, Anton Khirnov wrote:
>>> Quoting James Almer (2024-01-21 18:47:43)
>>>> On 1/21/2024 2:29 PM, Anton Khirnov wrote:
>>>>> Honestly this whole new API strikes me as massively overthinking it. All
>>>>> you should need to describe an arbitrary partition of an image into
>>>>> sub-rectangles is an array of (x, y, width, height). Instead you're
>>>>> proposing a new public header, struct, three functions, multiple "tile
>>>>> types", and if I'm not mistaken it still cannot describe an arbitrary
>>>>> partitioning. Plus it's in libavutil for some reason, even though
>>>>> libavformat seems to be the only intended user.
>>>>>
>>>>> Is all this complexity really warranted?
>>>>
>>>> 1. It needs to be usable as a Stream Group type, so a struct is
>>>> required. Said struct needs an allocator unless we want to have its size
>>>> be part of the ABI. I can remove the free function, but then the caller
>>>> needs to manually free any internal data.
>>>
>>> If the struct lives in lavf and is always allocated as a part of
>>> AVStreamGroup then you don't need a public constructor/destructor and
>>> can still extend the struct.
>>
>> Yes, but that would be the case if it's only meant to be allocated by
>> AVStreamGroup and nothing else.
> 
> That is the case right now, no?
> 
> If that ever changes then the constructor can be added.
> 
>>>
>>>> 2. We need tile dimensions (Width and height) plus row and column count,
>>>> which give you the final size of the grid, then offsets x and y to get
>>>> the actual image within the grid meant for presentation.
>>>> 3. I want to support uniform tiles as well as variable tile dimensions,
>>>> hence multiple tile types. The latter currently has no use case, but
>>>> eventually might. I can if you prefer not include said type at first,
>>>> but i want to keep the union in place so it and other extensions can be
>>>> added.
>>>> 4. It's in lavu because its meant to be generic. It can also be used to
>>>> transport tiling and cropping information as stream and packet side
>>>> data, which can't depend on something defined in lavf.
>>>
>>> When would you have tiling information associated with a specific
>>> stream?
>>
>> Can't think of an example for tiling, but i can for cropping. If you
>> insist on not reusing this for non-HEIF cropping usage in mp4/matroska,
>> then ok, I'll move it to lavf.
> 
> I still don't see why should it be a good idea to use this struct for
> generic container cropping. It feels very much like a hammer in search
> of a nail.

Because once we support container cropping, we will be defining a 
stream/packet side data type that will contain a subset of the fields 
from this struct.

If we reuse this struct, we can export a clap box as an AVTileGrid (Or i 
can rename it to AVImageGrid, and tile to subrectangle) either as the 
stream group tile grid specific parameters if HEIF, or as stream side 
data otherwise.

> 
>>>
>>>> And what do you mean with not supporting describing arbitrary
>>>> partitioning? Isn't that what variable tile dimensions achieve?
>>>
>>> IIUC your tiling scheme still assumes that the partitioning is by rows
>>> and columns. A completely generic partitioning could be irregular.
>>
>> A new tile type that doesn't define rows and columns can be added if
>> needed. But the current variable tile type can support things like grids
>> of two rows and two columns where the second row is effectively a single
>> tile, simply by setting the second tile in said row as having a width of 0.
> 
> The problem I see here is that every consumer of this struct then has to
> explicitly support every type, and adding a new type requires updating
> all callers. This seems unnecessary when "list of N rectangles" covers
> all possible partitionings.

Well, the variable type supports a list of N rectangles where each 
rectangle has arbitrary dimensions, and you can do things like having 
three tiles/rectangles that together still form a rectangle, while 
defining row and column count. So i don't personally see the need for a 
new type to begin with.

> 
> That does not mean you actually have to store it that way - the struct
> could be a list of N rectangles logically, while actually being
> represented more efficiently (in the same way a channel layout is always
> logically a list of channels, even though it's often represented by an
> uint64 rather than a malloced array).
> 
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/21/2024 4:29 PM, James Almer wrote:
> On 1/21/2024 4:02 PM, Anton Khirnov wrote:
>> Quoting James Almer (2024-01-21 19:38:50)
>>> On 1/21/2024 3:29 PM, Anton Khirnov wrote:
>>>> Quoting James Almer (2024-01-21 18:47:43)
>>>>> On 1/21/2024 2:29 PM, Anton Khirnov wrote:
>>>>>> Honestly this whole new API strikes me as massively overthinking 
>>>>>> it. All
>>>>>> you should need to describe an arbitrary partition of an image into
>>>>>> sub-rectangles is an array of (x, y, width, height). Instead you're
>>>>>> proposing a new public header, struct, three functions, multiple 
>>>>>> "tile
>>>>>> types", and if I'm not mistaken it still cannot describe an arbitrary
>>>>>> partitioning. Plus it's in libavutil for some reason, even though
>>>>>> libavformat seems to be the only intended user.
>>>>>>
>>>>>> Is all this complexity really warranted?
>>>>>
>>>>> 1. It needs to be usable as a Stream Group type, so a struct is
>>>>> required. Said struct needs an allocator unless we want to have its 
>>>>> size
>>>>> be part of the ABI. I can remove the free function, but then the 
>>>>> caller
>>>>> needs to manually free any internal data.
>>>>
>>>> If the struct lives in lavf and is always allocated as a part of
>>>> AVStreamGroup then you don't need a public constructor/destructor and
>>>> can still extend the struct.
>>>
>>> Yes, but that would be the case if it's only meant to be allocated by
>>> AVStreamGroup and nothing else.
>>
>> That is the case right now, no?
>>
>> If that ever changes then the constructor can be added.
>>
>>>>
>>>>> 2. We need tile dimensions (Width and height) plus row and column 
>>>>> count,
>>>>> which give you the final size of the grid, then offsets x and y to get
>>>>> the actual image within the grid meant for presentation.
>>>>> 3. I want to support uniform tiles as well as variable tile 
>>>>> dimensions,
>>>>> hence multiple tile types. The latter currently has no use case, but
>>>>> eventually might. I can if you prefer not include said type at first,
>>>>> but i want to keep the union in place so it and other extensions 
>>>>> can be
>>>>> added.
>>>>> 4. It's in lavu because its meant to be generic. It can also be 
>>>>> used to
>>>>> transport tiling and cropping information as stream and packet side
>>>>> data, which can't depend on something defined in lavf.
>>>>
>>>> When would you have tiling information associated with a specific
>>>> stream?
>>>
>>> Can't think of an example for tiling, but i can for cropping. If you
>>> insist on not reusing this for non-HEIF cropping usage in mp4/matroska,
>>> then ok, I'll move it to lavf.
>>
>> I still don't see why should it be a good idea to use this struct for
>> generic container cropping. It feels very much like a hammer in search
>> of a nail.
> 
> Because once we support container cropping, we will be defining a 
> stream/packet side data type that will contain a subset of the fields 
> from this struct.
> 
> If we reuse this struct, we can export a clap box as an AVTileGrid (Or i 
> can rename it to AVImageGrid, and tile to subrectangle) either as the 
> stream group tile grid specific parameters if HEIF, or as stream side 
> data otherwise.
> 
>>
>>>>
>>>>> And what do you mean with not supporting describing arbitrary
>>>>> partitioning? Isn't that what variable tile dimensions achieve?
>>>>
>>>> IIUC your tiling scheme still assumes that the partitioning is by rows
>>>> and columns. A completely generic partitioning could be irregular.
>>>
>>> A new tile type that doesn't define rows and columns can be added if
>>> needed. But the current variable tile type can support things like grids
>>> of two rows and two columns where the second row is effectively a single
>>> tile, simply by setting the second tile in said row as having a width 
>>> of 0.
>>
>> The problem I see here is that every consumer of this struct then has to
>> explicitly support every type, and adding a new type requires updating
>> all callers. This seems unnecessary when "list of N rectangles" covers
>> all possible partitionings.
> 
> Well, the variable type supports a list of N rectangles where each 
> rectangle has arbitrary dimensions, and you can do things like having 
> three tiles/rectangles that together still form a rectangle, while 
> defining row and column count. So i don't personally see the need for a 
> new type to begin with.

I could remove the types and the union altogether and leave only the 
array even for uniform tiles if you think that simplifies the API, but 
seems like a waste of memory to allocate a rows x cols array of ints 
just to have the same value written for every entry.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-21 20:29:58)
> On 1/21/2024 4:02 PM, Anton Khirnov wrote:
> > I still don't see why should it be a good idea to use this struct for
> > generic container cropping. It feels very much like a hammer in search
> > of a nail.
> 
> Because once we support container cropping, we will be defining a 
> stream/packet side data type that will contain a subset of the fields 
> from this struct.

AVCodecParameters is a subset of AVCodecContext. By this logic we should
use AVCodecContext everywhere instead of AVCodecParameters.

There are also issues with storing it in packet side data since it
can contain pointers to malloced data.

> If we reuse this struct, we can export a clap box as an AVTileGrid (Or i 
> can rename it to AVImageGrid, and tile to subrectangle) either as the 
> stream group tile grid specific parameters if HEIF, or as stream side 
> data otherwise.

How does that make the API better? It seems highly counterintuitive to
me to export container cropping information in a struct designed for
tiling.

> > 
> >>>
> >>>> And what do you mean with not supporting describing arbitrary
> >>>> partitioning? Isn't that what variable tile dimensions achieve?
> >>>
> >>> IIUC your tiling scheme still assumes that the partitioning is by rows
> >>> and columns. A completely generic partitioning could be irregular.
> >>
> >> A new tile type that doesn't define rows and columns can be added if
> >> needed. But the current variable tile type can support things like grids
> >> of two rows and two columns where the second row is effectively a single
> >> tile, simply by setting the second tile in said row as having a width of 0.
> > 
> > The problem I see here is that every consumer of this struct then has to
> > explicitly support every type, and adding a new type requires updating
> > all callers. This seems unnecessary when "list of N rectangles" covers
> > all possible partitionings.
> 
> Well, the variable type supports a list of N rectangles where each 
> rectangle has arbitrary dimensions, and you can do things like having 
> three tiles/rectangles that together still form a rectangle, while 
> defining row and column count. So i don't personally see the need for a 
> new type to begin with.

I don't see how is that supposed to work. E.g. consider the following
partitioning:
┌─┬────┬─┐
│ │    ├─┤
├─┤    │ │
│ ├────┤ │
└─┴────┴─┘

How would you represent it in this API?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-21 22:03:10)
> I could remove the types and the union altogether and leave only the
> array even for uniform tiles if you think that simplifies the API, but
> seems like a waste of memory to allocate a rows x cols array of ints
> just to have the same value written for every entry.

My point is that the API can abstract away the details of how the data
is stored. You could have it always behave like a list of N rectangles,
while only storing as much information as is needed.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/22/2024 7:32 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 20:29:58)
>> On 1/21/2024 4:02 PM, Anton Khirnov wrote:
>>> I still don't see why should it be a good idea to use this struct for
>>> generic container cropping. It feels very much like a hammer in search
>>> of a nail.
>>
>> Because once we support container cropping, we will be defining a
>> stream/packet side data type that will contain a subset of the fields
>> from this struct.
> 
> AVCodecParameters is a subset of AVCodecContext. By this logic we should
> use AVCodecContext everywhere instead of AVCodecParameters.
> 
> There are also issues with storing it in packet side data since it
> can contain pointers to malloced data.
> 
>> If we reuse this struct, we can export a clap box as an AVTileGrid (Or i
>> can rename it to AVImageGrid, and tile to subrectangle) either as the
>> stream group tile grid specific parameters if HEIF, or as stream side
>> data otherwise.
> 
> How does that make the API better? It seems highly counterintuitive to
> me to export container cropping information in a struct designed for
> tiling.
> 
>>>
>>>>>
>>>>>> And what do you mean with not supporting describing arbitrary
>>>>>> partitioning? Isn't that what variable tile dimensions achieve?
>>>>>
>>>>> IIUC your tiling scheme still assumes that the partitioning is by rows
>>>>> and columns. A completely generic partitioning could be irregular.
>>>>
>>>> A new tile type that doesn't define rows and columns can be added if
>>>> needed. But the current variable tile type can support things like grids
>>>> of two rows and two columns where the second row is effectively a single
>>>> tile, simply by setting the second tile in said row as having a width of 0.
>>>
>>> The problem I see here is that every consumer of this struct then has to
>>> explicitly support every type, and adding a new type requires updating
>>> all callers. This seems unnecessary when "list of N rectangles" covers
>>> all possible partitionings.
>>
>> Well, the variable type supports a list of N rectangles where each
>> rectangle has arbitrary dimensions, and you can do things like having
>> three tiles/rectangles that together still form a rectangle, while
>> defining row and column count. So i don't personally see the need for a
>> new type to begin with.
> 
> I don't see how is that supposed to work. E.g. consider the following
> partitioning:
> ┌─┬────┬─┐
> │ │    ├─┤
> ├─┤    │ │
> │ ├────┤ │
> └─┴────┴─┘
> 
> How would you represent it in this API?

That's two rows and three columns. Lets assume the smallest rectangle 
there is 1x1:

1x2 2x3 1x1
1x2 2x1 1x3

Meaning

tile_width[] = { 1, 2, 1, 1, 2, 1 };
tile_height[] = { 2, 3, 1, 2, 1, 3 };

As long as the sum of widths on every row and the sum of heights on 
every column is the same (To ensure you get a rectangle), it can be 
represented.

If what you're trying to say is "What about offsets?", they can be 
inferred based on dimensions and position of previous tiles within the grid.
I don't think adding yet another array to store offsets is worth it. But 
maybe a helper function that will build it?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/22/2024 7:32 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 20:29:58)
>> On 1/21/2024 4:02 PM, Anton Khirnov wrote:
>>> I still don't see why should it be a good idea to use this struct for
>>> generic container cropping. It feels very much like a hammer in search
>>> of a nail.
>>
>> Because once we support container cropping, we will be defining a
>> stream/packet side data type that will contain a subset of the fields
>> from this struct.
> 
> AVCodecParameters is a subset of AVCodecContext. By this logic we should
> use AVCodecContext everywhere instead of AVCodecParameters.
> 
> There are also issues with storing it in packet side data since it
> can contain pointers to malloced data.

Ah, that's true. At least if i remove the union.

> 
>> If we reuse this struct, we can export a clap box as an AVTileGrid (Or i
>> can rename it to AVImageGrid, and tile to subrectangle) either as the
>> stream group tile grid specific parameters if HEIF, or as stream side
>> data otherwise.
> 
> How does that make the API better? It seems highly counterintuitive to
> me to export container cropping information in a struct designed for
> tiling.

Or you could consider it a struct designed to represent a group of 
images combined in a grid, for the purpose of extracting a subrectangle 
for presentation. How many images there are is defined by the rows and 
cols fields, and it can be 1.

> 
>>>
>>>>>
>>>>>> And what do you mean with not supporting describing arbitrary
>>>>>> partitioning? Isn't that what variable tile dimensions achieve?
>>>>>
>>>>> IIUC your tiling scheme still assumes that the partitioning is by rows
>>>>> and columns. A completely generic partitioning could be irregular.
>>>>
>>>> A new tile type that doesn't define rows and columns can be added if
>>>> needed. But the current variable tile type can support things like grids
>>>> of two rows and two columns where the second row is effectively a single
>>>> tile, simply by setting the second tile in said row as having a width of 0.
>>>
>>> The problem I see here is that every consumer of this struct then has to
>>> explicitly support every type, and adding a new type requires updating
>>> all callers. This seems unnecessary when "list of N rectangles" covers
>>> all possible partitionings.
>>
>> Well, the variable type supports a list of N rectangles where each
>> rectangle has arbitrary dimensions, and you can do things like having
>> three tiles/rectangles that together still form a rectangle, while
>> defining row and column count. So i don't personally see the need for a
>> new type to begin with.
> 
> I don't see how is that supposed to work. E.g. consider the following
> partitioning:
> ┌─┬────┬─┐
> │ │    ├─┤
> ├─┤    │ │
> │ ├────┤ │
> └─┴────┴─┘
> 
> How would you represent it in this API?
> 
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/22/2024 7:38 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 22:03:10)
>> I could remove the types and the union altogether and leave only the
>> array even for uniform tiles if you think that simplifies the API, but
>> seems like a waste of memory to allocate a rows x cols array of ints
>> just to have the same value written for every entry.
> 
> My point is that the API can abstract away the details of how the data
> is stored. You could have it always behave like a list of N rectangles,
> while only storing as much information as is needed.

The struct needs tile rows, tile columns, tile dimensions, output 
dimensions, and cropping information. I don't see how it could get smaller.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-22 12:59:52)
> > 
> > I don't see how is that supposed to work. E.g. consider the following
> > partitioning:
> > ┌─┬────┬─┐
> > │ │    ├─┤
> > ├─┤    │ │
> > │ ├────┤ │
> > └─┴────┴─┘
> > 
> > How would you represent it in this API?
> 
> That's two rows and three columns. Lets assume the smallest rectangle 
> there is 1x1:
> 
> 1x2 2x3 1x1
> 1x2 2x1 1x3
> 
> Meaning
> 
> tile_width[] = { 1, 2, 1, 1, 2, 1 };
> tile_height[] = { 2, 3, 1, 2, 1, 3 };
> 
> As long as the sum of widths on every row and the sum of heights on 
> every column is the same (To ensure you get a rectangle), it can be 
> represented.
> 
> If what you're trying to say is "What about offsets?", they can be 
> inferred based on dimensions and position of previous tiles within the grid.
> I don't think adding yet another array to store offsets is worth it. But 
> maybe a helper function that will build it?

This seems horribly obfuscated to me. Why do the users of this API have
to deal with all this? Why are the notions of "tile rows", "tile
columns", and "tiles" necessary?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[Anton Khirnov]

Quoting James Almer (2024-01-22 13:08:43)
> Or you could consider it a struct designed to represent a group of 
> images combined in a grid, for the purpose of extracting a subrectangle 
> for presentation. How many images there are is defined by the rows and 
> cols fields, and it can be 1.

It still very much sounds like you wrote a hammer and now everything is
starting to look like a nail. What is the advantage of using such a
compelex struct to export 4 unsigned integers?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/25/2024 2:17 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-22 13:08:43)
>> Or you could consider it a struct designed to represent a group of
>> images combined in a grid, for the purpose of extracting a subrectangle
>> for presentation. How many images there are is defined by the rows and
>> cols fields, and it can be 1.
> 
> It still very much sounds like you wrote a hammer and now everything is
> starting to look like a nail. What is the advantage of using such a
> compelex struct to export 4 unsigned integers?

None other than not adding another struct. But i already dropped this 
idea and moved everything to lavf, like you asked.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/25/2024 2:13 PM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-22 12:59:52)
>>>
>>> I don't see how is that supposed to work. E.g. consider the following
>>> partitioning:
>>> ┌─┬────┬─┐
>>> │ │    ├─┤
>>> ├─┤    │ │
>>> │ ├────┤ │
>>> └─┴────┴─┘
>>>
>>> How would you represent it in this API?
>>
>> That's two rows and three columns. Lets assume the smallest rectangle
>> there is 1x1:
>>
>> 1x2 2x3 1x1
>> 1x2 2x1 1x3
>>
>> Meaning
>>
>> tile_width[] = { 1, 2, 1, 1, 2, 1 };
>> tile_height[] = { 2, 3, 1, 2, 1, 3 };
>>
>> As long as the sum of widths on every row and the sum of heights on
>> every column is the same (To ensure you get a rectangle), it can be
>> represented.
>>
>> If what you're trying to say is "What about offsets?", they can be
>> inferred based on dimensions and position of previous tiles within the grid.
>> I don't think adding yet another array to store offsets is worth it. But
>> maybe a helper function that will build it?
> 
> This seems horribly obfuscated to me. Why do the users of this API have
> to deal with all this? Why are the notions of "tile rows", "tile
> columns", and "tiles" necessary?

It aligns with how HEIF (and even bitstream codecs like AV1 internally) 
define a grid of images put together. Since this will be mainly used for 
the former, it seems like the best way to propagate such information.

Strictly speaking, the "tile" dimensions are available as part of each 
stream in the group, so now that i made the struct not be generic for 
lavu, i can remove the arrays. But how to place the images in a grid 
still requires the concept of rows and columns.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".