search

NVIDIA / VideoProcessingFramework

Set of Python bindings to C++ libraries which provides full HW acceleration for video decoding, encoding and GPU-accelerated color space and pixel format conversions
Apache License 2.0
1.31k stars 233 forks source link

surface.CopyFrom(other) is backwards? #385

Open rcode6 opened 2 years ago

rcode6 commented 2 years ago

Describe the bug For two PySurface objects surface1 and surface2, surface1.CopyFrom(surface2,context,stream) copies the data from surface1 into surface2, but from the function name I'd expect that surface2 was copied into surface1.

To Reproduce Steps to reproduce the behavior:

  1. Take two PySurface objects
  2. use CopyFrom

Expected behavior I expected surface1.CopyFrom(surface2,context,stream) to copy from surface2 into surface1

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Additional context Just a semantics issue, if this is the intended behavior I can just note it in my code and continue using as-is.

contentis commented 1 year ago

@rarzumanyan, what do you think? Makes sense to me.

gedoensmax commented 1 year ago

We might want to deprecate this API and implement something similar to torch.clone https://pytorch.org/docs/stable/generated/torch.clone.html for use going forward enabling something like surface1 = surface2.Clone(context,stream)

rcode6 commented 1 year ago

There's already a surface.Clone function, which I actually used to use. I switched to surface.CopyFrom since I can skip a costly memory allocation step with a re-used surface. Ends up being 2x faster processing in my use case, so I'd really prefer surface.CopyFrom is not deprecated.

theHamsta commented 1 year ago

This should probably follow numpy conventions https://numpy.org/doc/stable/reference/generated/numpy.copyto.html . Also Numpy uses the inverse semantics where the preposition refers to the noun following it. We could switch to Numpy's .copyto/.copy naming while deprecating the old function due to it's confusing semantics. .CloneTo would of course also possible to keep the current naming style.