[FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider HW acceleration method when selecting decoder

[Xiang, Haihao]

On Mon, 2022-08-01 at 11:12 +0200, Anton Khirnov wrote:
> Hi,
> the concept is generally ok, but I have a few comments on the
> implementation.
> 
> Quoting Xiang, Haihao (2022-07-25 06:30:51)
> > +static const AVCodec *choose_decoder2(InputStream *ist, OptionsContext *o,
> > AVFormatContext *s, AVStream *st)
> > +{
> > +    char *codec_name = NULL;
> > +
> > +    MATCH_PER_STREAM_OPT(codec_names, str, codec_name, s, st);
> > +    if (codec_name) {
> > +        const AVCodec *codec = find_codec_or_die(codec_name, st->codecpar-
> > >codec_type, 0);
> > +        st->codecpar->codec_id = codec->id;
> > +        if (recast_media && st->codecpar->codec_type != codec->type)
> > +            st->codecpar->codec_type = codec->type;
> > +        return codec;
> > +    } else {
> > +        if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > +            ist->hwaccel_id == HWACCEL_GENERIC &&
> > +            ist->hwaccel_device_type != AV_HWDEVICE_TYPE_NONE) {
> > +            const AVCodec *p;
> 
> 'p' is a rather weird name for an AVCodec instance, I'd expect 'c' or
> 'codec' or something like that
> 
> > +            void *i = 0;
> 
> NULL, it's a pointer
> 
> > +
> > +            while ((p = av_codec_iterate(&i))) {
> > +                int j;
> > +
> > +                if (p->id != st->codecpar->codec_id ||
> > +                    !av_codec_is_decoder(p) ||
> > +                    !avcodec_get_hw_config(p, 0))
> > +                    continue;
> > +
> > +                for (j = 0; ;j++) {
> 
> for (int j = 0; config = avcodec_get_hw_config(p, j); j++)
> 
> gets rid of the explicit check in the loop and also
> avcodec_get_hw_config() in the condition above
> 
> > +                    const AVCodecHWConfig *config =
> > avcodec_get_hw_config(p, j);
> > +
> > +                    if (!config)
> > +                        break;
> > +
> > +                    if (config->device_type == ist->hwaccel_device_type)
> > +                        return p;
> > +                }
> > +            }
> > +        }
> > +
> > +        return avcodec_find_decoder(st->codecpar->codec_id);
> > +    }
> > +}
> > +
> >  /* Add all the streams from the given input file to the global
> >   * list of input streams. */
> >  static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
> > @@ -973,7 +1015,7 @@ static void add_input_streams(OptionsContext *o,
> > AVFormatContext *ic)
> >              ist->hwaccel_pix_fmt = AV_PIX_FMT_NONE;
> >          }
> >  
> > -        ist->dec = choose_decoder(o, ic, st);
> > +        ist->dec = choose_decoder2(ist, o, ic, st);
> 
> I don't like adding a new function that partially duplicates
> choose_decoder() and has a non-descriptive name. Just extend
> choose_decoder() by passing hwaccel_id and hwaccel_device_type to it.

Thanks for the comments, fixed in v4

-Haihao