Define CpuDeviceInterface

[dvrogozh]

Fixes: #619

• Commit defines CpuDeviceInterface and moves video *OnCPU methods from SingleStreamDecoder to it.
• Audio *OnCPU methods left in SingleStreamDecoder
• convertAVFrameToFrameOutput API of DeviceInterface was changed to allow passing AVRational timeBase required to initialize ffmpeg filter graph

CC: @scotts, @NicolasHug

[dvrogozh]

@scotts, @NicolasHug : here is a next PR where I attempt to define CpuDeviceInterface and move *OnCPU methods into it. At the moment I did not physically move methods to the dedicated CpuDeviceInterface.cpp file. I think this way it will be easier to review the difference and discuss. I suggest to make physical move in the follow up PR once we agree on implementation.

Pay attention on the need to have AVRational time_base to initialize ffmpeg filter graph. This value is a property of AVStream. At the moment I've handled it by changing DeviceInterface constructor and creation methods to take in this value. Alternatively this value also presents in AVCodecContext which DeviceInterface has access to via initializeContext method. However, on the moment of the call context is not yet fully initialized as it's preparational method and potentially timeBase might be initialized later. So, I decided to better change DeviceInterface constructor API instead to handle that.

I also see that you are adding audio support. I did not move audio *OnCPU methods to the DeviceInterface. Do you want to move them as well? If yes, we need to think how to handle the case that Audio is handled only by CPU at the moment.

[scotts]

@dvrogozh, I'm going to be on vacation next week, so I won't be able to provide a full review until after I get back. Some initial comments:

1. I'll defer to @NicolasHug on audio, but my take is that we don't touch it for now. We can tackle it when/if we implement accelerator support for audio.
2. Adding the timeBase as a parameter to the DeviceInterface seems reasonable as we need it, but I'll have a more substantial take when I take a closer look.
3. I appreciate leaving the functions in-place for the first round of reviews, but we'll want to move the functions in this PR. Both because we will want to review the code as it's actually going to be, and because splitting up refactors into multiple PRs has a cost for us when we import the code internally.

[NicolasHug]

Thanks for the PR @dvrogozh . I agree with both of you to keep audio where it currently is for now.

[dvrogozh]

I appreciate leaving the functions in-place for the first round of reviews, but we'll want to move the functions in this PR.

I added the commit in this PR which moves functions. Overall there are currently 3 commits (you might wish to review commit by commit or the entire diff - your choice):

1. First commit creates CpuDeviceInterface and modifies functions in-place (this commit is what I have initially posted)
2. Second commit moves some more frame related APIs to Frame.h and Frame.cpp (such as getHeightAndWidthFrom* and allocateEmptyHWCTensor). This is needed because at least some of these functions are actually used across device interface implementations, cuda and cpu wise. This allows to avoid including SingleStreamDecoder.h in the device interface implementations entirely. Note that this commit can be separated to another PR if needed.
3. Third commit moves functions from SingleStreamDecoder.cpp to CpuDeviceInterface.cpp. No modifications in functions - just moving them to .cpp. The only additional modifications in this commit are adding/removing header files to fix compilation after moving the code.

[dvrogozh]

Rebased on top of latest main, resolved minor conflict (in cmakelists.txt). @scotts : please, help to review once you will have some spare time.

[scotts]

@dvrogozh, this is great! My one request: I think it's a little better for us to pass the timeBase to convertAVFrameToFrameOutput() rather than to the constructors for the devices. We don't actually need the timeBase until we request to convert a frame. I think this makes the device construction more clear. It will have to be a [[maybe_unused]] parameter for non-CPU devices, but they also weren't using the class member either.

Thanks for working on this!

[dvrogozh]

My one request: I think it's a little better for us to pass the timeBase to convertAVFrameToFrameOutput()

Done (last commit in PR, previous not changed).

[dvrogozh]

Run ${CONDA_RUN} ffmpeg -buildconf
ffmpeg: error while loading shared libraries: libXau.so.6: cannot open shared object file: No such file or directory
ERROR conda.cli.main_run:execute(125): `conda run ffmpeg -buildconf` failed. (See above for error)
Error: Process completed with exit code 127.

That seems to be some CI issue. Was the runner recently changed/updated?

[scotts]

@dvrogozh, those are some unrelated CI issues that we haven't resolved. This looks great, thanks for working on this and making the changes! I'm approving now, and then I'll merge early next week.

[facebook-github-bot]

@scotts has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.