[FFmpeg-devel] [PATCH] Extract QP from h264 encoded videos

Mark Thompson sw at jkqxz.net
Mon Jul 29 22:48:33 EEST 2019


On 29/07/2019 19:09, Juan De León wrote:
> Changes to libavcodec, hope this addresses all your comments.
> Cheers.

This doesn't belong in the commit message.

What does belong here would be some commentary on why you want this feature.

> Signed-off-by: Juan De León <juandl at google.com>
> ---
>  libavutil/Makefile              |   2 +
>  libavutil/frame.h               |   6 ++
>  libavutil/quantization_params.c |  41 ++++++++++++
>  libavutil/quantization_params.h | 106 ++++++++++++++++++++++++++++++++
>  4 files changed, 155 insertions(+)
>  create mode 100644 libavutil/quantization_params.c
>  create mode 100644 libavutil/quantization_params.h
> 
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 8a7a44e4b5..be5e5d831f 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -60,6 +60,7 @@ HEADERS = adler32.h                                                     \
>            pixdesc.h                                                     \
>            pixelutils.h                                                  \
>            pixfmt.h                                                      \
> +          quantization_params.o 										\

Object file in the list of headers?

Also broken spacing - tabs are not allowed in C files.

>            random_seed.h                                                 \
>            rc4.h                                                         \
>            rational.h                                                    \
> @@ -140,6 +141,7 @@ OBJS = adler32.o                                                        \
>         parseutils.o                                                     \
>         pixdesc.o                                                        \
>         pixelutils.o                                                     \
> +       quantization_params.o                                            \
>         random_seed.o                                                    \
>         rational.o                                                       \
>         reverse.o                                                        \
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 5d3231e7bb..d48ccf342f 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -179,6 +179,12 @@ enum AVFrameSideDataType {
>       * array element is implied by AVFrameSideData.size / AVRegionOfInterest.self_size.
>       */
>      AV_FRAME_DATA_REGIONS_OF_INTEREST,
> +    /**
> +     * To extract quantization parameters from supported decoders.
> +     * The data stored is AVQuantizationParamsArray type, described in
> +     * libavuitls/quantization_params.h
> +     */
> +    AV_FRAME_DATA_QUANTIZATION_PARAMS,
>  };
>  
>  enum AVActiveFormatDescription {
> diff --git a/libavutil/quantization_params.c b/libavutil/quantization_params.c
> new file mode 100644
> index 0000000000..28b08ebe19
> --- /dev/null
> +++ b/libavutil/quantization_params.c
> @@ -0,0 +1,41 @@
> +/*
> + * 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/quantization_params.h"
> +
> +static const char* const QP_NAMES_H264[] = {"qp"};
> +
> +static const char* const QP_NAMES_VP9[] = {"qydc", "qyac", "quvdc", "quvac",
> +                                           "qiydc", "qiyac", "qiuvdc", "qiuvac"};
> +
> +static const char* const QP_NAMES_AV1[] = {"qydc", "qyac", "qudc", "quac", "qvdc", "qvac",
> +                                      "qiydc", "qiyac", "qiudc", "qiuac", "qivdc", "qivac"};
> +
> +char* get_qp_str(enum AVCodecID codec_id, int index)

The values this function is returning are pointers to const char.

> +{
> +    switch (codec_id) {
> +        case AV_CODEC_ID_H264:
> +            return QP_NAMES_H264[index];
> +        case AV_CODEC_ID_VP9:
> +            return QP_NAMES_VP9[index];
> +        case AV_CODEC_ID_AV1:
> +            return QP_NAMES_AV1[index];
> +        default:
> +            return NULL;
> +    }
> +}
> diff --git a/libavutil/quantization_params.h b/libavutil/quantization_params.h
> new file mode 100644
> index 0000000000..7a3daeaae5
> --- /dev/null
> +++ b/libavutil/quantization_params.h
> @@ -0,0 +1,106 @@
> +/*
> + * 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_QUANTIZATION_PARAMS_H
> +#define AVUTIL_QUANTIZATION_PARAMS_H
> +
> +#include "libavcodec/avcodec.h"

This is not allowed - libavutil cannot depend on libavcodec.

> +
> +// Max size for the type array: QP_ARR_SIZE_AV1 = 12
> +#define MAX_SIZE 12

Everything in header files must be namespaced.  (This name will clash everywhere - indeed, there are multiple instances of this name in ffmpeg already.)

> +
> +/**
> + * Data structure for extracting Quantization Parameters, codec independent
> + */
> +typedef struct AVQuantizationParams {
> +    /**
> +     * x and y coordinates of the block in pixels
> +     */
> +    int x, y;

How do these values interact with cropping?

> +    /**
> +     * width and height of the block in pixels
> +     */
> +    int w, h;
> +    /**
> +     * qp array, indexed by type according to
> +     * the enum corresponding to the codec
> +     */
> +    int type[MAX_SIZE];

This name is confusing; it should probably at least suggest that it contains quantiser values.

> +} AVQuantizationParams;
> +
> +/**
> + * For storing an array of AVQuantization parameters and its size
> + * Used as AVFrameSideData
> + */
> +typedef struct AVQuantizationParamsArray {
> +    /**
> +     * AVQuantizationParams block array
> +     */
> +    AVQuantizationParams *qp_arr;

Side-data is reference counted, so how is this pointer managed?  More genrally, it would probably help to explain exactly how this is allocated and who will be responsible for freeing it.

> +    /**
> +     * size of the array
> +     */
> +    int nb_blocks;
> +
> +    enum AVCodecID codec_id;
> +} AVQuantizationParamsArray;
> +
> +/**
> + * Get the string describing the qp type for the given codec
> + */
> +char* get_qp_str(enum AVCodecID codec_id, int index);

Requires namespacing.

The name feels very unhelpful (characters are not a scarce resource when defining external API).

It should also have a note on failure conditions - as written above, it returns NULL for invalid codec_id and invoked undefined behaviour for invalid index.

> +
> +/**
> + * Enums for different codecs to store qp in the type array
> + * Each enum must have an array of strings describing each field
> + * declared in quantization_params.c
> + */
> +enum QP_ARR_INDEXES_FOR_H264 {
> +        QP_H264 = 0,            // qp value

What value specifically does this refer to?  QP_Y or QP'_Y for the given block?

> +        QP_ARR_SIZE_H264        // used for allocating memory

Given that you have chroma quantiser values for the other codecs, why not for H.264?

> +};
> +
> +enum QP_ARR_INDEXES_FOR_VP9 {
> +        QP_YDC_VP9 = 0,
> +        QP_YAC_VP9,
> +        QP_UVDC_VP9,
> +        QP_UVAC_VP9,
> +        QP_INDEX_YDC_VP9,
> +        QP_INDEX_YAC_VP9,
> +        QP_INDEX_UVDC_VP9,
> +        QP_INDEX_UVAC_VP9,

Can you explain exactly what each of these are with reference to the standard?  (Something like the return value of get_[ad]c_quant(N) for the block?)

It might help not to call them QPs, since that isn't a term used in VP9.

> +        QP_ARR_SIZE_VP9
> +};
> +
> +enum QP_ARR_INDEXES_FOR_AV1 {
> +        QP_YDC_AV1 = 0,
> +        QP_YAC_AV1,
> +        QP_UDC_AV1,
> +        QP_UAC_AV1,
> +        QP_VDC_AV1,
> +        QP_VAC_AV1,
> +        QP_INDEX_YDC_AV1,
> +        QP_INDEX_YAC_AV1,
> +        QP_INDEX_UDC_AV1,
> +        QP_INDEX_UAC_AV1,
> +        QP_INDEX_VDC_AV1,
> +        QP_INDEX_VAC_AV1,

Precise explanation of all of these needed too.

> +        QP_ARR_SIZE_AV1
> +};

Everything in these enums also requires namespacing.

> +
> +#endif /* AVUTIL_QUANTIZATION_PARAMS_H */
> 

What is the proposed consumer of this?  It might help if you provided that as well (a filter?).

- Mark


More information about the ffmpeg-devel mailing list