| Message ID | 20190801234830.88569-1-juandl@google.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Aug 01, 2019 at 04:48:32PM -0700, Juan De León wrote: > Fixed That would end in the commit message which is probably unintended > > design doc: https://docs.google.com/document/d/1WClt3EqhjwdGXhEw386O0wfn3IBQ1Ib-_5emVM1gbnA/edit?usp=sharing > > Signed-off-by: Juan De León <juandl@google.com> > --- > libavutil/Makefile | 2 + > libavutil/frame.h | 6 ++ > libavutil/quantization_params.c | 42 +++++++++++++ > libavutil/quantization_params.h | 101 ++++++++++++++++++++++++++++++++ > 4 files changed, 151 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..be1a9c3a9c 100644 > --- a/libavutil/Makefile > +++ b/libavutil/Makefile > @@ -60,6 +60,7 @@ HEADERS = adler32.h \ > pixdesc.h \ > pixelutils.h \ > pixfmt.h \ > + quantization_params.h \ > 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..b64fd9c02c 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 is stored as AVQuantizationParamsArray type, described in > + * libavuitl/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..152be71ba7 > --- /dev/null > +++ b/libavutil/quantization_params.c > @@ -0,0 +1,42 @@ > +/* > + * 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 <stddef.h> > + > +#include "libavutil/quantization_params.h" > + > +static const char* const QP_NAMES_H264[] = {"qp", "qpcb", "qpcr"}; > + > +static const char* const QP_NAMES_VP9[] = {"qyac", "qydc", "quvdc", "quvac", > + "qiyac", "qiydc", "qiuvdc", "qiuvac"}; > + > +static const char* const QP_NAMES_AV1[] = {"qyac", "qydc", "qudc", "quac", "qvdc", "qvac", > + "qiyac", "qiydc", "qiudc", "qiuac", "qivdc", "qivac"}; > + The meaning of the chars in the strings should maybe be described So generic code could interpret a new codecs qp values though for such generic interpretation more would be needed like which direction quality improves, if its linear or log scale, ... so maybe this is just out of scope of this initial implementation > +const char* av_get_qp_type_string(enum AVExtractQPSupportedCodecs codec_id, int index) > +{ > + switch (codec_id) { > + case AV_EXTRACT_QP_CODEC_ID_H264: > + return index < AV_QP_ARR_SIZE_H264 ? QP_NAMES_H264[index] :NULL; > + case AV_EXTRACT_QP_CODEC_ID_VP9: > + return index < AV_QP_ARR_SIZE_VP9 ? QP_NAMES_VP9[index] :NULL; > + case AV_EXTRACT_QP_CODEC_ID_AV1: > + return index < AV_QP_ARR_SIZE_AV1 ? QP_NAMES_AV1[index] :NULL; > + default: > + return NULL; > + } > +} index is checked for being too large but not for too small ( < 0 ) not sure that is intended [...] thx
On Thu, Aug 01, 2019 at 04:48:32PM -0700, Juan De León wrote: > Fixed > > design doc: https://docs.google.com/document/d/1WClt3EqhjwdGXhEw386O0wfn3IBQ1Ib-_5emVM1gbnA/edit?usp=sharing > > Signed-off-by: Juan De León <juandl@google.com> > --- > libavutil/Makefile | 2 + > libavutil/frame.h | 6 ++ > libavutil/quantization_params.c | 42 +++++++++++++ > libavutil/quantization_params.h | 101 ++++++++++++++++++++++++++++++++ > 4 files changed, 151 insertions(+) > create mode 100644 libavutil/quantization_params.c > create mode 100644 libavutil/quantization_params.h [...] > +/** > + * Data structure for extracting Quantization Parameters, codec independent > + */ > +typedef struct AVQuantizationParams { > + /** > + * x and y coordinates of the block in pixels > + */ > + int x, y; > + /** > + * width and height of the block in pixels > + * set to 0 for the last block in the array > + */ > + int w, h; > + /** > + * qp array, indexed by type according to > + * the enum corresponding to the codec > + */ > + int qp_type[AV_QP_ARR_SIZE_AV1]; What happens if a future codec needs more than AV1 ? i think this would not be extendible without breaking ABI [...]
On Sat, Aug 3, 2019 at 12:15 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > +const char* av_get_qp_type_string(enum AVExtractQPSupportedCodecs > codec_id, int index) > > +{ > > + switch (codec_id) { > > + case AV_EXTRACT_QP_CODEC_ID_H264: > > + return index < AV_QP_ARR_SIZE_H264 ? QP_NAMES_H264[index] > :NULL; > > + case AV_EXTRACT_QP_CODEC_ID_VP9: > > + return index < AV_QP_ARR_SIZE_VP9 ? QP_NAMES_VP9[index] > :NULL; > > + case AV_EXTRACT_QP_CODEC_ID_AV1: > > + return index < AV_QP_ARR_SIZE_AV1 ? QP_NAMES_AV1[index] > :NULL; > > + default: > > + return NULL; > > + } > > +} > > index is checked for being too large but not for too small ( < 0 ) > not sure that is intended > Added a check for (index < 0) to return NULL before the switch, will submit in new patch. On Sat, Aug 3, 2019 at 12:36 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > + if (h->avctx->debug & FF_DEBUG_EXTRACTQP) { > > + int mb_height = h->height / 16; > > + int mb_width = h->width / 16; > > has this been tested with files which have dimensions not a multiple of 16 > ? > Yes, tested with a 640x360 video, width and height here correspond to the coded dimension, which are always multiples of 16 so I believe this should not be a problem. typedef struct H264Context <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=337&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523H264Context%252523c%252523dEG_os146C2&gsn=H264Context&ct=xref_usages> { ... /* coded dimensions -- 16 * mb w/h */ int width <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=359&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523ZImaFPq8HKEGWVaoII7Cf0AKkOcO2Z5yD-AjKA2sPy0&gsn=width&ct=xref_usages>, height <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=359&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523-tOCyFteI1_t9m4iN9IbhvTDxCRW6aBjm8eEw4zgfno&gsn=height&ct=xref_usages>; ... > + if (!sd) { > > + return AVERROR(ENOMEM); > > buffer leaks here > I updated it to allocate an array of AVQuantizationParams in the side data so that it can all be freed with a single call. I will submit the new patch when the patch for libavutil is approved. On Sat, Aug 3, 2019 at 12:45 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > + int qp_type[AV_QP_ARR_SIZE_AV1]; What happens if a future codec needs more than AV1 ? > i think this would not be extendible without breaking ABI Would a macro for largest size be better in this case? I will make that change in a new patch also.
On Mon, Aug 05, 2019 at 12:16:08PM -0700, Juan De León wrote: > On Sat, Aug 3, 2019 at 12:15 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > > +const char* av_get_qp_type_string(enum AVExtractQPSupportedCodecs > > codec_id, int index) > > > +{ > > > + switch (codec_id) { > > > + case AV_EXTRACT_QP_CODEC_ID_H264: > > > + return index < AV_QP_ARR_SIZE_H264 ? QP_NAMES_H264[index] > > :NULL; > > > + case AV_EXTRACT_QP_CODEC_ID_VP9: > > > + return index < AV_QP_ARR_SIZE_VP9 ? QP_NAMES_VP9[index] > > :NULL; > > > + case AV_EXTRACT_QP_CODEC_ID_AV1: > > > + return index < AV_QP_ARR_SIZE_AV1 ? QP_NAMES_AV1[index] > > :NULL; > > > + default: > > > + return NULL; > > > + } > > > +} > > > > index is checked for being too large but not for too small ( < 0 ) > > not sure that is intended > > > Added a check for (index < 0) to return NULL before the switch, will submit > in new patch. > > > On Sat, Aug 3, 2019 at 12:36 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > > + if (h->avctx->debug & FF_DEBUG_EXTRACTQP) { > > > + int mb_height = h->height / 16; > > > + int mb_width = h->width / 16; > > > > has this been tested with files which have dimensions not a multiple of 16 > > ? > > > Yes, tested with a 640x360 video, width and height here correspond to the > coded dimension, which are always multiples of 16 so I believe this should > not be a problem. > typedef struct H264Context > <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=337&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523H264Context%252523c%252523dEG_os146C2&gsn=H264Context&ct=xref_usages> > { > ... > > /* coded dimensions -- 16 * mb w/h */ int width > <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=359&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523ZImaFPq8HKEGWVaoII7Cf0AKkOcO2Z5yD-AjKA2sPy0&gsn=width&ct=xref_usages>, > height <https://cs.corp.google.com/piper///depot/google3/third_party/ffmpeg/src/libavcodec/h264dec.h?l=359&gs=kythe%253A%252F%252Fgoogle3%253Flang%253Dc%25252B%25252B%253Fpath%253Dthird_party%252Fffmpeg%252Fsrc%252Flibavcodec%252Fh264dec.h%2523-tOCyFteI1_t9m4iN9IbhvTDxCRW6aBjm8eEw4zgfno&gsn=height&ct=xref_usages>; > ... > > > + if (!sd) { > > > + return AVERROR(ENOMEM); > > > > buffer leaks here > > > I updated it to allocate an array of AVQuantizationParams in the side data > so that it can all be freed with a single call. I will submit the new patch > when the patch for libavutil is approved. > > > On Sat, Aug 3, 2019 at 12:45 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > + int qp_type[AV_QP_ARR_SIZE_AV1]; > > What happens if a future codec needs more than AV1 ? > > i think this would not be extendible without breaking ABI > > Would a macro for largest size be better in this case? I will make that > change in a new patch also. a macro would still require a major version bump as its a ABI change, if it changes. To fix this would probably require a deeper change, we could also of course just leave it and accept that as the maximum till the next ABI major bump thanks [...]
On Mon, Aug 5, 2019 at 1:07 PM Michael Niedermayer <michael@niedermayer.cc> wrote > a macro would still require a major version bump as its a ABI change, if > it changes. > To fix this would probably require a deeper change, we could also > of course just leave it and accept that as the maximum till the next > ABI major bump > I added the macro to at least make it clearer, I don't see any other cleaner solution at the moment. Please review my latest patch and if there are any more concerns please let me know.
diff --git a/libavutil/Makefile b/libavutil/Makefile index 8a7a44e4b5..be1a9c3a9c 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -60,6 +60,7 @@ HEADERS = adler32.h \ pixdesc.h \ pixelutils.h \ pixfmt.h \ + quantization_params.h \ 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..b64fd9c02c 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 is stored as AVQuantizationParamsArray type, described in + * libavuitl/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..152be71ba7 --- /dev/null +++ b/libavutil/quantization_params.c @@ -0,0 +1,42 @@ +/* + * 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 <stddef.h> + +#include "libavutil/quantization_params.h" + +static const char* const QP_NAMES_H264[] = {"qp", "qpcb", "qpcr"}; + +static const char* const QP_NAMES_VP9[] = {"qyac", "qydc", "quvdc", "quvac", + "qiyac", "qiydc", "qiuvdc", "qiuvac"}; + +static const char* const QP_NAMES_AV1[] = {"qyac", "qydc", "qudc", "quac", "qvdc", "qvac", + "qiyac", "qiydc", "qiudc", "qiuac", "qivdc", "qivac"}; + +const char* av_get_qp_type_string(enum AVExtractQPSupportedCodecs codec_id, int index) +{ + switch (codec_id) { + case AV_EXTRACT_QP_CODEC_ID_H264: + return index < AV_QP_ARR_SIZE_H264 ? QP_NAMES_H264[index] :NULL; + case AV_EXTRACT_QP_CODEC_ID_VP9: + return index < AV_QP_ARR_SIZE_VP9 ? QP_NAMES_VP9[index] :NULL; + case AV_EXTRACT_QP_CODEC_ID_AV1: + return index < AV_QP_ARR_SIZE_AV1 ? QP_NAMES_AV1[index] :NULL; + default: + return NULL; + } +} diff --git a/libavutil/quantization_params.h b/libavutil/quantization_params.h new file mode 100644 index 0000000000..23f4311293 --- /dev/null +++ b/libavutil/quantization_params.h @@ -0,0 +1,101 @@ +/* + * 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 + +/* + * Supported decoders for extraction and filter + */ +enum AVExtractQPSupportedCodecs { + AV_EXTRACT_QP_CODEC_ID_H264 = 0, + AV_EXTRACT_QP_CODEC_ID_VP9, + AV_EXTRACT_QP_CODEC_ID_AV1, +}; + +/** + * 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 AVQPArrIndexesH264 { + AV_QP_Y_H264 = 0, // QPy + AV_QP_U_H264, // QPcb + AV_QP_V_H264, // QPcr + AV_QP_ARR_SIZE_H264 +}; + +enum AVQPArrIndexesVP9 { + AV_QP_YAC_VP9 = 0, // get_dc_quant[][base_q_idx] - in spec + AV_QP_YDC_VP9, // get_dc_quant[][base_q_idx+delta_q_y_dc] + AV_QP_UVDC_VP9, // get_dc_quant[][base_q_idx+delta_q_uv_dc] + AV_QP_UVAC_VP9, // get_ac_quant[][base_q_idx+delta_q_uv_ac] + AV_QP_INDEX_YAC_VP9, // base_q_idx + AV_QP_INDEX_YDC_VP9, // base_q_idx+delta_q_y_dc + AV_QP_INDEX_UVDC_VP9, // base_q_idx+delta_q_uv_dc + AV_QP_INDEX_UVAC_VP9, // base_q_idx+delta_q_uv_ac + AV_QP_ARR_SIZE_VP9 +}; + +enum AVQPArrIndexesAV1 { + AV_QP_YAC_AV1 = 0, // dc_q(base_q_idx) - in spec + AV_QP_YDC_AV1, // dc_q(base_q_idx+DeltaQYDc) + AV_QP_UDC_AV1, // dc_q(base_q_idx+DeltaQUDc) + AV_QP_UAC_AV1, // dc_q(base_q_idx+DeltaQUAc) + AV_QP_VDC_AV1, // dc_q(base_q_idx+DeltaQVDc) + AV_QP_VAC_AV1, // dc_q(base_q_idx+DeltaQVAc) + AV_QP_INDEX_YAC_AV1, // base_q_idx + AV_QP_INDEX_YDC_AV1, // base_q_idx+DeltaQYDc + AV_QP_INDEX_UDC_AV1, // base_q_idx+DeltaQUDc + AV_QP_INDEX_UAC_AV1, // base_q_idx+DeltaQUAc + AV_QP_INDEX_VDC_AV1, // base_q_idx+DeltaQVDc + AV_QP_INDEX_VAC_AV1, // base_q_idx+DeltaQVAc + AV_QP_ARR_SIZE_AV1 +}; + +/** + * Data structure for extracting Quantization Parameters, codec independent + */ +typedef struct AVQuantizationParams { + /** + * x and y coordinates of the block in pixels + */ + int x, y; + /** + * width and height of the block in pixels + * set to 0 for the last block in the array + */ + int w, h; + /** + * qp array, indexed by type according to + * the enum corresponding to the codec + */ + int qp_type[AV_QP_ARR_SIZE_AV1]; + /** + * Stores an id corresponding to one of the supported codecs + */ + enum AVExtractQPSupportedCodecs codec_id; +} AVQuantizationParams; + +/** + * Get the string describing the qp type for the given codec + */ +const char* av_get_qp_type_string(enum AVExtractQPSupportedCodecs codec_id, int index); + +#endif /* AVUTIL_QUANTIZATION_PARAMS_H */
Fixed design doc: https://docs.google.com/document/d/1WClt3EqhjwdGXhEw386O0wfn3IBQ1Ib-_5emVM1gbnA/edit?usp=sharing Signed-off-by: Juan De León <juandl@google.com> --- libavutil/Makefile | 2 + libavutil/frame.h | 6 ++ libavutil/quantization_params.c | 42 +++++++++++++ libavutil/quantization_params.h | 101 ++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+) create mode 100644 libavutil/quantization_params.c create mode 100644 libavutil/quantization_params.h