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 <mich...@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 <mich...@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 <mich...@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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
