[FFmpeg-devel] libavutil: AVEncodeInfo data structures

AVEncodeInfoFrame data structure to store as AVFrameSideData of type
AV_FRAME_DATA_ENCODE_INFO.
The structure stores quantization index for each plane, DC/AC deltas
for luma and chroma planes, and an array of AVEncodeInfoBlock type
denoting position, size, and delta quantizer for each block in the
frame.
Can be extended to support extraction of other block information.

Signed-off-by: Juan De León <juandl@google.com>
---
 libavutil/Makefile      |   2 +
 libavutil/encode_info.c |  68 +++++++++++++++++++++++++
 libavutil/encode_info.h | 110 ++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.c       |   1 +
 libavutil/frame.h       |   7 +++
 5 files changed, 188 insertions(+)
 create mode 100644 libavutil/encode_info.c
 create mode 100644 libavutil/encode_info.h

Juan De León (12019-08-13):
> AVEncodeInfoFrame data structure to store as AVFrameSideData of type
> AV_FRAME_DATA_ENCODE_INFO.
> The structure stores quantization index for each plane, DC/AC deltas
> for luma and chroma planes, and an array of AVEncodeInfoBlock type
> denoting position, size, and delta quantizer for each block in the
> frame.
> Can be extended to support extraction of other block information.
> 
> Signed-off-by: Juan De León <juandl@google.com>
> ---
>  libavutil/Makefile      |   2 +
>  libavutil/encode_info.c |  68 +++++++++++++++++++++++++
>  libavutil/encode_info.h | 110 ++++++++++++++++++++++++++++++++++++++++
>  libavutil/frame.c       |   1 +
>  libavutil/frame.h       |   7 +++
>  5 files changed, 188 insertions(+)
>  create mode 100644 libavutil/encode_info.c
>  create mode 100644 libavutil/encode_info.h
> 
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 57e6e3d7e8..37cfb099e9 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -24,6 +24,7 @@ HEADERS = adler32.h                                                     \
>            dict.h                                                        \
>            display.h                                                     \
>            downmix_info.h                                                \
> +          encode_info.h                                                 \
>            encryption_info.h                                             \
>            error.h                                                       \
>            eval.h                                                        \
> @@ -111,6 +112,7 @@ OBJS = adler32.o                                                        \
>         dict.o                                                           \
>         display.o                                                        \
>         downmix_info.o                                                   \
> +       encode_info.o                                                    \
>         encryption_info.o                                                \
>         error.o                                                          \
>         eval.o                                                           \
> diff --git a/libavutil/encode_info.c b/libavutil/encode_info.c
> new file mode 100644
> index 0000000000..ffc43f2c19
> --- /dev/null
> +++ b/libavutil/encode_info.c
> @@ -0,0 +1,68 @@
> +/*
> + * 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/encode_info.h"
> +#include "libavutil/mem.h"
> +
> +static int init_encode_info_data(AVEncodeInfoFrame *info, int nb_blocks) {
> +    info->nb_blocks = nb_blocks;
> +    info->block_size = sizeof(AVEncodeInfoBlock);

> +    info->blocks_offset = offsetof(AVEncodeInfoFrame, blocks);

You can use sizeof(AVEncodeInfoFrame) and dispense with the blocks final
array entirely.

> +
> +    for(int i = 0; i < AV_NUM_DATA_POINTERS; i++)
> +        info->plane_q[i] = -1;
> +
> +    return 0;
> +}
> +
> +AVEncodeInfoFrame *av_encode_info_alloc(int nb_blocks)
> +{
> +    AVEncodeInfoFrame *ptr;
> +    //AVEncodeInfoFrame already allocates size for one element of AVEncodeInfoBlock
> +    size_t size = sizeof(AVEncodeInfoFrame) + sizeof(AVEncodeInfoBlock)*FFMIN(nb_blocks - 1, 0);
> +
> +    if (nb_blocks < 0 || size >= INT_MAX)
> +        return NULL;
> +
> +    ptr = av_mallocz(size);
> +    if (!ptr)
> +        return NULL;
> +
> +    init_encode_info_data(ptr, nb_blocks);
> +
> +    return ptr;
> +}
> +
> +AVEncodeInfoFrame *av_encode_info_create_side_data(AVFrame *frame, int nb_blocks)
> +{
> +    size_t size = sizeof(AVEncodeInfoFrame) + sizeof(AVEncodeInfoBlock)*FFMIN(nb_blocks - 1, 0);
> +
> +    if (nb_blocks < 0 || size >= INT_MAX)
> +        return NULL;
> +
> +    AVFrameSideData *sd = av_frame_new_side_data(frame,
> +                                                 AV_FRAME_DATA_ENCODE_INFO,
> +                                                 size);
> +    if (!sd)
> +        return NULL;
> +
> +    memset(sd->data, 0, size);
> +    init_encode_info_data((AVEncodeInfoFrame*)sd->data, nb_blocks);
> +
> +    return (AVEncodeInfoFrame*)sd->data;
> +}
> diff --git a/libavutil/encode_info.h b/libavutil/encode_info.h
> new file mode 100644
> index 0000000000..864e42bdcf
> --- /dev/null
> +++ b/libavutil/encode_info.h
> @@ -0,0 +1,110 @@
> +/*
> + * 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_ENCODE_INFO_H
> +#define AVUTIL_ENCODE_INFO_H
> +
> +#include "libavutil/frame.h"
> +
> +/**
> + * Data structure for extracting block data, stored as an array in AVEncodeInfoFrame.
> + */
> +typedef struct AVEncodeInfoBlock{
> +    /**
> +     * Distance in luma pixels from the top-left corner of the visible frame
> +     * to the top-left corner of the block.
> +     * Can be negative if top/right padding is present on the coded frame.
> +     */
> +    int src_x, src_y;
> +    /**
> +     * Width and height of the block in luma pixels.
> +     */
> +    int w, h;
> +    /**
> +     * Delta quantization index for the block.
> +     */
> +    int delta_q;
> +} AVEncodeInfoBlock;
> +
> +/**
> + * Frame encoding info, used as AVFrameSideData. Data in this structure concerns
> + * the whole frame.
> + * Additional entries may be added without bumping major before nb_blocks,
> + * so using the accessor function av_encode_info_get_block() is recommended.
> + */
> +typedef struct AVEncodeInfoFrame {
> +    /**
> +     * Base plane quantizer for the frame, set to -1 when value is unsupported.
> +     */
> +    int plane_q[AV_NUM_DATA_POINTERS];
> +    /**
> +     * DC/AC quantizer index delta, set to -1 when value is value unsupported.
> +     */
> +    int ac_q, dc_q;
> +    /**
> +     * DC/AC chroma quantizer index delta, set to -1 when value is value unsupported.
> +     */
> +    int ac_chroma_q, dc_chroma_q;
> +    /**
> +     * Number of blocks in the array, may be 0.
> +     */
> +    int nb_blocks;
> +    /**
> +     * Offset in this structure at which blocks begin in bytes. May not match
> +     * offsetof(AVEncodeInfoFrame, blocks).
> +     */
> +    size_t blocks_offset;
> +    /*
> +     * Size of each block in bytes. May not match sizeof(AVEncodeInfoBlock).
> +     */
> +    size_t block_size;
> +
> +    /*
> +     * Array of blocks, with a total size of block_size*nb_blocks, the [1]
> +     * is meant for compatibility with C++.
> +     */
> +    AVEncodeInfoBlock blocks[1];
> +} AVEncodeInfoFrame;
> +
> +/*
> + * Gets the block at the specified {@code idx}
> + */
> +static inline AVEncodeInfoBlock *av_encode_info_get_block(AVEncodeInfoFrame *info, int idx)
> +{

> +    if (!info || idx >= info->nb_blocks || idx < 0)
> +        return NULL;

How valid is it for applications to call with idx outside the range?

> +
> +    return (AVEncodeInfoBlock *)((uint8_t *)info + info->blocks_offset + idx*info->block_size);
> +}
> +
> +/**
> + * Allocates memory for AVEncodeInfoFrame plus an array of
> + * {@code nb_blocks} AVEncodeInfoBlock and initializes the variables.
> + * Can be freed with a normal av_free() call.
> + */
> +AVEncodeInfoFrame *av_encode_info_alloc(int nb_blocks);
> +
> +/**
> + * Allocates memory for AVEncodeInfoFrame plus an array of
> + * {@code nb_blocks} AVEncodeInfoBlock in the given AVFrame {@code frame}
> + * as AVFrameSideData of type AV_FRAME_DATA_ENCODE_INFO
> + * and initializes the variables.
> + */
> +AVEncodeInfoFrame *av_encode_info_create_side_data(AVFrame *frame, int nb_blocks);
> +
> +#endif /* AVUTIL_ENCODE_INFO_H */
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index dcf1fc3d17..65c25e6cd7 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -842,6 +842,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>  #endif
>      case AV_FRAME_DATA_DYNAMIC_HDR_PLUS: return "HDR Dynamic Metadata SMPTE2094-40 (HDR10+)";
>      case AV_FRAME_DATA_REGIONS_OF_INTEREST: return "Regions Of Interest";
> +    case AV_FRAME_DATA_ENCODE_INFO:                 return "AVEncodeInfo";
>      }
>      return NULL;
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 5d3231e7bb..ec112c5d15 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -179,6 +179,13 @@ enum AVFrameSideDataType {
>       * array element is implied by AVFrameSideData.size / AVRegionOfInterest.self_size.
>       */
>      AV_FRAME_DATA_REGIONS_OF_INTEREST,
> +    /**
> +     * Extract frame and block encode info from supported decoders. The data
> +     * stored is an AVEncodeInfoFrame type, which contains an array of
> +     * AVEncodeInfoBlock. Described in libavuitls/encode_info.h
> +     * Can be allocated in the frame directly with av_encode_info_create_side_data().
> +     */
> +    AV_FRAME_DATA_ENCODE_INFO,
>  };
>  
>  enum AVActiveFormatDescription {

Regards,

On Tue, Aug 13, 2019 at 2:49 PM Nicolas George <george@nsup.org> wrote:

> > +    info->blocks_offset = offsetof(AVEncodeInfoFrame, blocks);
>
> You can use sizeof(AVEncodeInfoFrame) and dispense with the blocks final
> array entirely.
>
The array is there so that the structure isn't opaque, it should be
accessed with the function.


> > +    if (!info || idx >= info->nb_blocks || idx < 0)
> > +        return NULL;
>
> How valid is it for applications to call with idx outside the range?

They shouldn't but I figure it's better to return NULL than to get
undefined behaviour.

Juan De León (12019-08-13):
> The array is there so that the structure isn't opaque, it should be
> accessed with the function.

I realize you need it, but not for the reason you say. It is needed for
alignment: if blocks needs more alignment than info, info+sizeof(info)
is not a valid pointer for blocks.

> > > +    if (!info || idx >= info->nb_blocks || idx < 0)
> > > +        return NULL;
> > How valid is it for applications to call with idx outside the range?
> They shouldn't but I figure it's better to return NULL than to get
> undefined behaviour.

In that case, I think is is better practice to document:

	Index must be between 0 and nb_blocks

and check with an assert, forcing the application programmer fix their
code immediately.

Most code will likely use idx from a loop counter, where it cannot be
outside the range, and for the few other cases, the caller can do the
check if necessary.

Also, make the fields that cannot be negative unsigned, and you can drop
the <0 test.

Regards,

On 8/13/2019 7:29 PM, Nicolas George wrote:
> Juan De León (12019-08-13):
>> The array is there so that the structure isn't opaque, it should be
>> accessed with the function.
> 
> I realize you need it, but not for the reason you say. It is needed for
> alignment: if blocks needs more alignment than info, info+sizeof(info)
> is not a valid pointer for blocks.
> 
>>>> +    if (!info || idx >= info->nb_blocks || idx < 0)
>>>> +        return NULL;
>>> How valid is it for applications to call with idx outside the range?
>> They shouldn't but I figure it's better to return NULL than to get
>> undefined behaviour.
> 
> In that case, I think is is better practice to document:
> 
> 	Index must be between 0 and nb_blocks
> 
> and check with an assert, forcing the application programmer fix their
> code immediately.
> 
> Most code will likely use idx from a loop counter, where it cannot be
> outside the range, and for the few other cases, the caller can do the
> check if necessary.
> 
> Also, make the fields that cannot be negative unsigned, and you can drop
> the <0 test.
> 
> Regards,

I'm fairly sure this was discussed before, but invalid arguments
shouldn't crash an user's application. They even have their own
standardized errno value for this purpose.
asserts() are to catch bugs in our code, not in theirs. Returning a NULL
pointer is the preferred behavior.

On Tue, Aug 13, 2019 at 4:49 PM James Almer <jamrial@gmail.com> wrote:

> I'm fairly sure this was discussed before, but invalid arguments
> shouldn't crash an user's application. They even have their own
> standardized errno value for this purpose.
> asserts() are to catch bugs in our code, not in theirs. Returning a NULL
> pointer is the preferred behavior.
>

Thank you for the feedback, I will update the patch accordingly.
Also I noticed I was using FFMIN instead of FFMAX here:
size_t size = sizeof(AVEncodeInfoFrame) +
sizeof(AVEncodeInfoBlock)*FFMIN(nb_blocks
- 1, 0);

If anyone has any more feedback or wants to discuss the patch I'll also be
available in the IRC channel.

James Almer (12019-08-13):
> I'm fairly sure this was discussed before, but invalid arguments
> shouldn't crash an user's application. They even have their own
> standardized errno value for this purpose.
> asserts() are to catch bugs in our code, not in theirs. Returning a NULL
> pointer is the preferred behavior.

I do not agree. Asserts are to catch all bugs that can be catched
statically. There is no sense in making some bugs harder to catch just
because they involve separate developers.

Nobody can predict whether a disk will make a I/O error, whether there
will be enough memory, etc., that kind of error MUST be handled
gracefully.

On the other hand, it is easy to make sure that a buffer given to read()
is not NULL, and therefore it is very acceptable to just crash when that
happens.

EINVAL is for cases where the acceptable value cannot be easily
predicted by the caller. For example, setting an unsupported sample rate
for the sound device: the caller could check in advance, but that would
be cumbersome.

Now, please tell me, according to you, "idx is not smaller than
nb_blocks", is it more akin to a disk I/O error, a NULL buffer or an
invalid sample rate?

Another argument:

Instead of providing utility code, we could just document the
arithmetic. In that case, the application would have code that says, in
essence: "block = info + offset + idx * block_size". No check. What
would happen if idx was too big? Not a graceful error: a crash, or
worse.

The assert mimics that, in a more convenient way since it gives the
reason and line number, and does not allow the bug to devolve into
something more serious than a crash.

Regards,

On Wed, Aug 14, 2019 at 12:10 AM Nicolas George <george@nsup.org> wrote:

> James Almer (12019-08-13):
> > I'm fairly sure this was discussed before, but invalid arguments
> > shouldn't crash an user's application. They even have their own
> > standardized errno value for this purpose.
> > asserts() are to catch bugs in our code, not in theirs. Returning a NULL
> > pointer is the preferred behavior.
>
> I do not agree. Asserts are to catch all bugs that can be catched
> statically. There is no sense in making some bugs harder to catch just
> because they involve separate developers.
>
> Nobody can predict whether a disk will make a I/O error, whether there
> will be enough memory, etc., that kind of error MUST be handled
> gracefully.
>
> On the other hand, it is easy to make sure that a buffer given to read()
> is not NULL, and therefore it is very acceptable to just crash when that
> happens.
>
> EINVAL is for cases where the acceptable value cannot be easily
> predicted by the caller. For example, setting an unsupported sample rate
> for the sound device: the caller could check in advance, but that would
> be cumbersome.
>
> Now, please tell me, according to you, "idx is not smaller than
> nb_blocks", is it more akin to a disk I/O error, a NULL buffer or an
> invalid sample rate?
>
> Another argument:
>
> Instead of providing utility code, we could just document the
> arithmetic. In that case, the application would have code that says, in
> essence: "block = info + offset + idx * block_size". No check. What
> would happen if idx was too big? Not a graceful error: a crash, or
> worse.
>
> The assert mimics that, in a more convenient way since it gives the
> reason and line number, and does not allow the bug to devolve into
> something more serious than a crash.
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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".


In that case, I believe documenting the size of the array and behaviour of
undefined indexes should be enough. Have the pointers return NULL,
and let the user handle the result, instead of stopping the execution.

I would prefer to discuss the actual data structure for now.

Juan De León (12019-08-14):
> In that case, I believe documenting the size of the array and behaviour of
> undefined indexes should be enough. Have the pointers return NULL,
> and let the user handle the result, instead of stopping the execution.

I disagree. Either drop the check altogether or make it an assert. A
NULL return is a compromise that is worse than either.

> I would prefer to discuss the actual data structure for now.

This is whai I can comment on.

Regards,