Skip to content

Define CpuDeviceInterface #636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

dvrogozh
Copy link
Contributor

Fixes: #619

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

CC: @scotts, @NicolasHug

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Apr 10, 2025
@dvrogozh
Copy link
Contributor Author

@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
Copy link
Contributor

scotts commented Apr 12, 2025

@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
Copy link
Member

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

Fixes: pytorch#619

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

Signed-off-by: Dmitry Rogozhkin <[email protected]>
@dvrogozh
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define CpuDevice class and move CPU specific methods (color conversion) to it
4 participants