Skip to content

Commit

Permalink
ffmpeg: remove superfluous custom cuvid hwaccel
Browse files Browse the repository at this point in the history 
It's a duplicate of the properly implemented nvdec libavcodec hwaccel

Reviewed-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: James Almer <jamrial@gmail.com>
  • Loading branch information
@jamrial
jamrial committed Mar 3, 2020
1 parent 7020900 commit 60b1f85
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 80 deletions.
1 change: 0 additions & 1 deletion fftools/Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ ALLAVPROGS = $(AVBASENAMES:%=%$(PROGSSUF)$(EXESUF))
ALLAVPROGS_G = $(AVBASENAMES:%=%$(PROGSSUF)_g$(EXESUF))

OBJS-ffmpeg += fftools/ffmpeg_opt.o fftools/ffmpeg_filter.o fftools/ffmpeg_hw.o
OBJS-ffmpeg-$(CONFIG_CUVID) += fftools/ffmpeg_cuvid.o
OBJS-ffmpeg-$(CONFIG_LIBMFX) += fftools/ffmpeg_qsv.o
ifndef CONFIG_VIDEOTOOLBOX
OBJS-ffmpeg-$(CONFIG_VDA) += fftools/ffmpeg_videotoolbox.o
Expand Down
2 changes: 0 additions & 2 deletions fftools/ffmpeg.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ enum HWAccelID {
HWACCEL_GENERIC,
HWACCEL_VIDEOTOOLBOX,
HWACCEL_QSV,
HWACCEL_CUVID,
};

typedef struct HWAccel {
Expand Down Expand Up @@ -654,7 +653,6 @@ int ffmpeg_parse_options(int argc, char **argv);

int videotoolbox_init(AVCodecContext *s);
int qsv_init(AVCodecContext *s);
int cuvid_init(AVCodecContext *s);

HWDevice *hw_device_get_by_name(const char *name);
int hw_device_init_from_string(const char *arg, HWDevice **dev);
Expand Down
73 changes: 0 additions & 73 deletions fftools/ffmpeg_cuvid.c
View file

This file was deleted.

5 changes: 1 addition & 4 deletions fftools/ffmpeg_opt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,6 @@ const HWAccel hwaccels[] = {
#endif
#if CONFIG_LIBMFX
{ "qsv", qsv_init, HWACCEL_QSV, AV_PIX_FMT_QSV },
#endif
#if CONFIG_CUVID
{ "cuvid", cuvid_init, HWACCEL_CUVID, AV_PIX_FMT_CUDA },
#endif
{ 0 },
};
Expand Down Expand Up @@ -822,7 +819,7 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st);
if (hwaccel) {
// The NVDEC hwaccels use a CUDA device, so remap the name here.
if (!strcmp(hwaccel, "nvdec"))
if (!strcmp(hwaccel, "nvdec") || !strcmp(hwaccel, "cuvid"))
hwaccel = "cuda";

if (!strcmp(hwaccel, "none"))
Expand Down

23 comments on commit 60b1f85

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What have you done? Why you removed cuvid? cuvid is the only correct value for NVIDIA hardware accelerated video decoding. https://trac.ffmpeg.org/ticket/6989#comment:7 explains it very well. It is essential that NVDEC engine software interface is integrated with the CUDA driver, because the video surfaces are allocated by the CUDA driver and can be accessed from CUDA kernels for things like color space conversion. Also some other features are implemented in CUDA.

There is no other way you can use NVIDIA hardware accelerated video decoding besides cuvid.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what you are talking about, neither here nor in that ticket.
The generic hwaccel code for CUDA hwaccels is in proper shape and correctly sets up the required contexts. The old coda in ffmpeg.c pre-dates it and was removed because it is no longer needed and was causing issues. No functionality was lost or changed in the process.
I just tried all possible combinations of -hwaccel values and legacy cuvid and native nvdec decoding. They all worked perfectly fine.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is native nvdec decoding. The cuvid family is only a legacy standalone decoder that predates the native nvdec hwaccels.
Just doing ffmpeg -hwaccel cuda -i some_h264_file.mp4 -f null - will utilize it.

The only reason I haven't deprecated the cuvid decoders yet is because they expose some of the scaling/cropping/deinterlacing capabilities of the nvidia hardware, which are otherwise not available.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have no idea what you are talking about.
The cuvid decoder and nvdec hwaccels in ffmpeg both utilize the exact same nvidia API. The cuvid decoder is just from before nvidia renamed it to nvdec.
Also no idea what you mean by "outside of CUDA driver". FFmpeg is clearly not a driver, and using the APIs quite successfully.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue you refer to is not related to the commandline argument, but the decoder being unable to predict how many frames the downstream chain needs.
So the only actual issue I see there is that the maximum number of allocated frames being capped at 32 is too low of a maximum, and should be 128 or something like that.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it would be dead code. It's not in any condition and not in an unused function.

"cuda" is just what ffmpeg calls its generic CUDA hwaccel infrastructure. It created a cuda context and frames context for decoders/filters to use.
FFmpeg is not the nvidia driver.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the legacy cuvid decoders and the nvdec hwaccel work fine for me. I'm really confused what kind of issue you are hunting here.
First of all, cuvid_init was removed by this patch. There is no function left in the codebase with that name.
If you do not specify any -hwaccel cuda/nvdec/cuvid parameter to ffmpeg.c, the legacy cuvid standalone decoder will just create its own, copy back the frames to normal frames at the end, and calls it a day.
The nvdec hwaccel relies on being externally supplied with a hwframes context. When passing -hwaccel cuda to ffmpeg.c, the generic codebase will make sure an appropiate one is being created.
decode.c then moves on to call frame_params on the codec specific hwaccel, which sets a few codec specific parameters and then passes on the the generic nvdec.c function ff_nvdec_frame_params. This happens unconditionally. There is no way the fields you are complaining about will ever end up unset. Specially as decode.c itself adds on to the initial pool size based on some more factors.

@Nevcairiel
Copy link
Contributor

@Nevcairiel Nevcairiel commented on 60b1f85 Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of rambling about code out of context and a change you blame for everything not working, why don't you actually demonstrate a command line which does not actually work like its supposed to (and worked with the cuvid option)? And don't just link to some old and long rambling trac issue that supposedly explains it, show actual tangible repeatable examples.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/nvdec.c#L303 you are referring to solely exists for a potential API user to supply their own hw_frames_ctx.
The generic hwaccel path, which is now the only path available for CUDA hwaccel, will only create a hw_device_ctx. nvdec.c will thus call ff_decode_get_hw_frames_ctx, which will call avcodec_get_hw_frames_parameters which will call (via the nvdec codec specific frame_params wrapper) ff_nvdec_frame_params, where the desired initial pool size is set.
If an external hw_frames_ctx is supplied, it is expected to be properly setup. The old cuvid hwaccel code in ffmpeg.c did NOT set it up properly, but was only ever meant to run the standalone cuvid family of decoders. Hence why combining -hwaccel cuvid + nvdec decoding used to break.
Instead of patching up the old code to somehow work with it, just removing it entirely and relying on the generic codepath to do its thing works just as fine, and gets rid of needless code duplication.

@Nevcairiel
Copy link
Contributor

@Nevcairiel Nevcairiel commented on 60b1f85 Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only one with attitude in this thread are you. And I haven't seen a single constructive error report, only a lot of opionated criticism.

I asked for an actual error report, something that worked before this change, and something that doesn't work now - basically, a reason why this change is so bad, instead of only theoretical "cuvid is superior" talk. If noone can even point to anything of that sort, then it must not be so bad (nevermind that "cuvid", "nvdec" and "cuda" hwaccels all point to the same code in ffmpeg anyway).

@Nevcairiel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have actually been discussing adding a deprecation warning to the -hwaccel cuvid compat code, so its likely that it'll make it in there.

In the meantime, the mentioned patch above will restore previous behavior without having to manually specify the output format when using -hwaccel cuvid, but do note that this is considered deprecated and will go away eventually (and using it will then inform you of this, once those patches land).

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather it's nvdec's older and slightly uglier cousin or something. I mean it kinda makes sense to get rid of the cuvid 'hwaccel' if it isn't a real hwaccel at all.

cuvid stands for CUda + VIDeo. It's a part of CUDA driver API and is the only way to access NVIDIA video decoding hardware acceleration capabilities. There are no cousins or siblings. It's the only offspring.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official name of the decode API, as per Nvidia, is nvdec. When it first launched, it was called cuvid, and because of that all the functions of the API still carry the cuvid prefix.
It was officially renamed, and all documentation and information material you find from Nvidia refers to it as nvdec. For example https://developer.nvidia.com/video-encode-decode-gpu-support-matrix

nvenc was always called nvenc though, and never related to cuvid/nvdec from an API point of view.

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also BtbN's posts above which explain that cuvid was the name of a standalone decoder, which existed before the native nvdec hardware acceleration method.

There is no special "native" acceleration method. There is only one method to access video acceleration: via CUDA driver.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not understand what you are trying to argue against. Nobody has ever doubted that you access the hardware de/encoders via the Nvidia driver.

On top of that, video hwaccel is also not strictly related to CUDA. nvenc is only loosely tied to CUDA and can operate nearly without it other than context bringup.
And for decoding there is at least one other CUDA-free API per OS to utilize it.

Everything else is just about the name of the old cuvid, now nvdec, API. Of course it's the exact same API. It was just renamed, without changing the API in any way.

@Nevcairiel
Copy link
Contributor

@Nevcairiel Nevcairiel commented on 60b1f85 Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're talking about FFmpeg terminology here, not NVIDIA ones. All access to the hardware is obviously done using the same API calls, since there is only one (unless you call platform APIs like DXVA).

In FFmpeg:

  • "cuda hwaccel" is what powers all of these, its based on cuda memory buffers that hold the video image, cuvid hwaccel is just an alias for that
  • "nvdec" is the name of the implementation of the "modern" hwaccels in FFmpeg, which utilize the "cuda hwaccel" to transport the images to filters or to NVENC. You can use these on CLI with -hwaccel cuda -c:v h264
  • "cuvid decoders" are the old-style decoders in FFmpeg, which are stand-alone and not based on our default h264/hevc/... decoders, which makes them inferious in various aspects, notably in handling many types of metadata for HDR and such. You use these on CLI with -c:v h264_cuvid (and optional -hwaccel cuvid)

None of those names really relate to what is done under-the-hood, since thats obviously all quite similar using the same "NVDEC" (formerly CUVID) API.

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was officially renamed, and all documentation and information material you find from Nvidia refers to it as nvdec. For example https://developer.nvidia.com/video-encode-decode-gpu-support-matrix

The table at this link in the capabilities of the actual hardware on the chip, i.e. NVDEC and NVENC refer in this table to specific GPU hardware blocks. You can easily see it in the name of the columns. This table has nothing to do with API. For example, the API supports decoding of JPEG, but you won't find it in this table, because it is emulated in CUDA.

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're talking about FFmpeg terminology here, not NVIDIA ones.

Exactly my point. The access to the hardware is via CUDA driver. Therefore the right name is cuvid.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvidia disagrees with that, having renamed it to nvdec in every piece of documentation.
For API and ABI compat reasons, only the technical part of the API itself(i.e. function names, constants, structs, ...) retain the old name.

The only reason I linked that table is to show it's called nvdec, not cuvid or nvcuvid anymore. See also for example https://developer.nvidia.com/nvidia-video-codec-sdk

"NVDECODE API for video decode acceleration (formerly called NVCUVID API)"

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not understand what you are trying to argue against. Nobody has ever doubted that you access the hardware de/encoders via the Nvidia driver.

"Nvidia driver" usually refers to a display driver. CUDA driver has nothing to do with it. You can access NVDEC hardware via NVIDIA driver, but it's a proprietary API, which NVIDIA never is going to publish.

On top of that, video hwaccel is also not strictly related to CUDA

Only CUDA driver can give you access to the hardware acceleration.

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I linked that table is to show it's called nvdec, not cuvid

In this table "NVDEC" is the name of the GPU hardware block, which of course has nothing to do with CUDA.

@BtbN
Copy link
Member

@BtbN BtbN commented on 60b1f85 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvidia only has one big driver. Which provides a ton of different APIs.
So far I have never heard anyone referring to different parts of it as different drivers.
There is no separately installed CUDA-Driver. Only the one, big, Nvidia driver. Internals of the driver, which probably is split into separate parts, are of no concern for the normal direct API user and enduser.

I don't get what kind of weird terminology problems you are fighting about here, but I franky don't care anymore. If you find any actual issues, please open a ticket on trac about them, with commandlines to reproduce.

@intractabilis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like we need it to reverse enginneer it.

It's a nightmare to program, it won't make life easier. Besides, it can be different for different chips.

Please sign in to comment.