| Message ID | 20220725043051.9692-2-haihao.xiang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3,1/2] ffmpeg_opt: select a decoder after getting values for per-stream hwdec options | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Mon, 2022-07-25 at 12:30 +0800, Xiang, Haihao wrote: > From: Haihao Xiang <haihao.xiang@intel.com> > > Usually a HW decoder is expected when user specifies a HW acceleration > method via -hwaccel option, however the current implementation doesn't > take HW acceleration method into account, it is possible to select a SW > decoder. > > For example: > $ ffmpeg -hwaccel vaapi -i av1.mp4 -f null - > $ ffmpeg -hwaccel nvdec -i av1.mp4 -f null - > $ ffmpeg -hwaccel vdpau -i av1.mp4 -f null - > [...] > Stream #0:0 -> #0:0 (av1 (libdav1d) -> wrapped_avframe (native)) > > libdav1d is selected in this case even if vaapi, nvdec or vdpau is > specified. > > After applying this patch, the native av1 decoder (with vaapi, nvdec or > vdpau support) is selected for decoding(libdav1d is still used for > probing format). > $ ffmpeg -hwaccel vaapi -i av1.mp4 -f null - > $ ffmpeg -hwaccel nvdec -i av1.mp4 -f null - > $ ffmpeg -hwaccel vdpau -i av1.mp4 -f null - > [...] > Stream #0:0 -> #0:0 (av1 (native) -> wrapped_avframe (native)) > > Tested-by: Mario Roy <marioeroy@gmail.com> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > --- > fftools/ffmpeg_opt.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index 2771c5d993..614bfeea80 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -861,6 +861,48 @@ static const AVCodec *choose_decoder(OptionsContext *o, > AVFormatContext *s, AVSt > return avcodec_find_decoder(st->codecpar->codec_id); > } > > +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; > + void *i = 0; > + > + 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++) { > + 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); > ist->decoder_opts = filter_codec_opts(o->g->codec_opts, ist->st- > >codecpar->codec_id, ic, st, ist->dec); > > ist->reinit_filters = -1; Hi, libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is always used when running the command below even if user expects vaapi or other HW acceleration methods. $ ffmpeg -hwaccel vaapi -i av1.ivf -f null - A similar command works as expected for other codecs, e.g. GPU is used when running the command below (see http://ffmpeg.org/pipermail/ffmpeg-user/2022-July/055227.html) $ ffmpeg -y -hwaccel vdpau -i input_vp9.webm output.ts This patchset can ensure FFmpeg chooses a HW accelerated decoder when '-hwaccel arg' is specified in the command line. Is there any comment for this patchset ? Thanks Haihao
Hi, On Fri, Jul 29, 2022 at 4:38 PM Xiang, Haihao < haihao.xiang-at-intel.com@ffmpeg.org> wrote: > libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is always used > when > running the command below even if user expects vaapi or other HW > acceleration > methods. > I think that is a pretty serious issue. Don't we always want to prefer a hw decoder by default? I agree there should also be ways to force-select specific hw/sw decoders, but shouldn't hw be the default? Ronald
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Ronald S. Bultje > Sent: Friday, July 29, 2022 10:50 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider HW > acceleration method when selecting decoder > > Hi, > > On Fri, Jul 29, 2022 at 4:38 PM Xiang, Haihao < > haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > > libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is always > used > > when > > running the command below even if user expects vaapi or other HW > > acceleration > > methods. > > > > I think that is a pretty serious issue. Don't we always want to > prefer a hw > decoder by default? I agree there should also be ways to force-select > specific hw/sw decoders, but shouldn't hw be the default? I don't think that this would be reasonably possible in any way. There are a lot of questions which ffmpeg cannot answer, e.g.: - Which hwaccel to choose? - Which hwaccel is available? - Which parameters are required for selecting a device that is working? - Is the auto-selected device even capable to decode a certain input? (pixel format, bit depth, codec profile, frame size, ...) For the user who is creating the command line, it is important to be able to rely on what is going to happen. If they can't command lines will fail: - The outputs of hw decoders vary. Some output to hw format, some to sw format by default. You may need to specify hwaccel_output_format or use hwdownload - Depending on the hwaccel, a totally different set of filters may be required (not to speak of encoders) - How would you specify that you want a sw decoder instead? IMO, predictability is the topmost important behavioral aspect for users (be it humans of machines.. ;-) Best regards, softworkz
On Fri, 2022-07-29 at 09:46 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Ronald S. Bultje > > Sent: Friday, July 29, 2022 10:50 AM > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider HW > > acceleration method when selecting decoder > > > > Hi, > > > > On Fri, Jul 29, 2022 at 4:38 PM Xiang, Haihao < > > haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > > > > libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is always > > > > used > > > when > > > running the command below even if user expects vaapi or other HW > > > acceleration > > > methods. > > > > > > > I think that is a pretty serious issue. Don't we always want to > > prefer a hw > > decoder by default? I agree there should also be ways to force-select > > specific hw/sw decoders, but shouldn't hw be the default? > > I don't think that this would be reasonably possible in any way. > There are a lot of questions which ffmpeg cannot answer, e.g.: > > - Which hwaccel to choose? > - Which hwaccel is available? > - Which parameters are required for selecting a device > that is working? > - Is the auto-selected device even capable to decode a certain > input? > (pixel format, bit depth, codec profile, frame size, ...) > > For the user who is creating the command line, it is important to > be able to rely on what is going to happen. If they can't command > lines will fail: > > - The outputs of hw decoders vary. Some output to hw format, some > to sw format by default. You may need to specify hwaccel_output_format > or use hwdownload > - Depending on the hwaccel, a totally different set of filters > may be required (not to speak of encoders) > - How would you specify that you want a sw decoder instead? > > IMO, predictability is the topmost important behavioral aspect > for users (be it humans of machines.. ;-) I agree with you it would be better not to make hw the default if user doesn't ask for. However '-hwaccel arg' is used to specify hw acceleration method in the examples. According to doc ( https://github.com/FFmpeg/FFmpeg/blob/master/doc/ffmpeg.texi#L1260-L1262), a hwdecoder is expected. "@item -hwaccel[:@var{stream_specifier}] @var{hwaccel} (@emph{input,per-stream}) Use hardware acceleration to decode the matching stream(s). " This patch is to fix the issue that a SW decoder is chosen even if user is expecting a hw method, not to make hw the default in any case. Thanks Haihao
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Monday, August 1, 2022 3:53 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider HW > acceleration method when selecting decoder > > On Fri, 2022-07-29 at 09:46 +0000, Soft Works wrote: > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Ronald S. Bultje > > > Sent: Friday, July 29, 2022 10:50 AM > > > To: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider > HW > > > acceleration method when selecting decoder > > > > > > Hi, > > > > > > On Fri, Jul 29, 2022 at 4:38 PM Xiang, Haihao < > > > haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > > > > > > libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is > always > > > > > > used > > > > when > > > > running the command below even if user expects vaapi or other > HW > > > > acceleration > > > > methods. > > > > > > > > > > I think that is a pretty serious issue. Don't we always want to > > > prefer a hw > > > decoder by default? I agree there should also be ways to force- > select > > > specific hw/sw decoders, but shouldn't hw be the default? > > > > I don't think that this would be reasonably possible in any way. > > There are a lot of questions which ffmpeg cannot answer, e.g.: > > > > - Which hwaccel to choose? > > - Which hwaccel is available? > > - Which parameters are required for selecting a device > > that is working? > > - Is the auto-selected device even capable to decode a certain > > input? > > (pixel format, bit depth, codec profile, frame size, ...) > > > > For the user who is creating the command line, it is important to > > be able to rely on what is going to happen. If they can't command > > lines will fail: > > > > - The outputs of hw decoders vary. Some output to hw format, some > > to sw format by default. You may need to specify > hwaccel_output_format > > or use hwdownload > > - Depending on the hwaccel, a totally different set of filters > > may be required (not to speak of encoders) > > - How would you specify that you want a sw decoder instead? > > > > IMO, predictability is the topmost important behavioral aspect > > for users (be it humans of machines.. ;-) > > > I agree with you it would be better not to make hw the default if > user doesn't > ask for. However '-hwaccel arg' is used to specify hw acceleration > method in the > examples. According to doc ( > https://github.com/FFmpeg/FFmpeg/blob/master/doc/ffmpeg.texi#L1260- > L1262), a hwdecoder is expected. > > "@item -hwaccel[:@var{stream_specifier}] @var{hwaccel} > (@emph{input,per-stream}) > Use hardware acceleration to decode the matching stream(s). " > > This patch is to fix the issue that a SW decoder is chosen even if > user is > expecting a hw method, not to make hw the default in any case. > > Thanks > Haihao > Oh, I'm in no way opposed to your patch, I just talked about the idea of a general automatic selection of hardware decoders. I agree that the behavior for AV1 should be the same as for the other decoders. Best wishes, softworkz
On Mon, 2022-08-01 at 02:14 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Xiang, Haihao > > Sent: Monday, August 1, 2022 3:53 AM > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider HW > > acceleration method when selecting decoder > > > > On Fri, 2022-07-29 at 09:46 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > Ronald S. Bultje > > > > Sent: Friday, July 29, 2022 10:50 AM > > > > To: FFmpeg development discussions and patches <ffmpeg- > > > > devel@ffmpeg.org> > > > > Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] ffmpeg_opt: consider > > > > HW > > > > acceleration method when selecting decoder > > > > > > > > Hi, > > > > > > > > On Fri, Jul 29, 2022 at 4:38 PM Xiang, Haihao < > > > > haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > > > > > > > > libdav1d is the preferred AV1 decoder in FFmpeg, libdav1d is > > > > always > > > > > > > > used > > > > > when > > > > > running the command below even if user expects vaapi or other > > > > HW > > > > > acceleration > > > > > methods. > > > > > > > > > > > > > I think that is a pretty serious issue. Don't we always want to > > > > prefer a hw > > > > decoder by default? I agree there should also be ways to force- > > > > select > > > > specific hw/sw decoders, but shouldn't hw be the default? > > > > > > I don't think that this would be reasonably possible in any way. > > > There are a lot of questions which ffmpeg cannot answer, e.g.: > > > > > > - Which hwaccel to choose? > > > - Which hwaccel is available? > > > - Which parameters are required for selecting a device > > > that is working? > > > - Is the auto-selected device even capable to decode a certain > > > input? > > > (pixel format, bit depth, codec profile, frame size, ...) > > > > > > For the user who is creating the command line, it is important to > > > be able to rely on what is going to happen. If they can't command > > > lines will fail: > > > > > > - The outputs of hw decoders vary. Some output to hw format, some > > > to sw format by default. You may need to specify > > > > hwaccel_output_format > > > or use hwdownload > > > - Depending on the hwaccel, a totally different set of filters > > > may be required (not to speak of encoders) > > > - How would you specify that you want a sw decoder instead? > > > > > > IMO, predictability is the topmost important behavioral aspect > > > for users (be it humans of machines.. ;-) > > > > > > I agree with you it would be better not to make hw the default if > > user doesn't > > ask for. However '-hwaccel arg' is used to specify hw acceleration > > method in the > > examples. According to doc ( > > https://github.com/FFmpeg/FFmpeg/blob/master/doc/ffmpeg.texi#L1260- > > L1262), a hwdecoder is expected. > > > > "@item -hwaccel[:@var{stream_specifier}] @var{hwaccel} > > (@emph{input,per-stream}) > > Use hardware acceleration to decode the matching stream(s). " > > > > This patch is to fix the issue that a SW decoder is chosen even if > > user is > > expecting a hw method, not to make hw the default in any case. > > > > Thanks > > Haihao > > > > Oh, I'm in no way opposed to your patch, I just talked about the > idea of a general automatic selection of hardware decoders. > Sorry for misreading your email and thanks for your clarifying. > I agree that the behavior for AV1 should be the same as for the other > decoders. Thanks, I will push it if no more comment in a few days. - Haihao
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.
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
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 2771c5d993..614bfeea80 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -861,6 +861,48 @@ static const AVCodec *choose_decoder(OptionsContext *o, AVFormatContext *s, AVSt return avcodec_find_decoder(st->codecpar->codec_id); } +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; + void *i = 0; + + 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++) { + 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); ist->decoder_opts = filter_codec_opts(o->g->codec_opts, ist->st->codecpar->codec_id, ic, st, ist->dec); ist->reinit_filters = -1;