Honor preAllocatedOutputTensor with filtergraph
#295
Conversation
facebook-github-bot
added
the
CLA Signed
|
Can you add a TODO to write a test that confirms the data_ptr in the pre-allocated tensor is the same as the return value in both color-conversion library cases? Otherwise this can regress and we would not know any better. Doesn't have to be this diff. You can do it later. |
| // decoded frame directly into `preAllocatedtensor.data_ptr()`. That's not | ||
| // possible with filtegraph, leading to an extra copy. |
There was a problem hiding this comment.
I don't think it's impossible with filtergraph -- we just don't know and I haven't investigated this further.
If ffmpeg allows us to pass in pre-allocated frames to filtergraph we can do this more efficiently for filtergraph too. I would add a TODO to investigate and fix this for filtergraph later.
test/decoders/VideoDecoderTest.cpp
Outdated
| auto preAllocatedOutputTensor = torch::empty({270, 480, 3}, {torch::kUInt8}); | ||
|
|
||
| std::unique_ptr<VideoDecoder> ourDecoder = | ||
| createDecoderFromPath(path, GetParam()); |
There was a problem hiding this comment.
So compilation fails on this line because I assume GetParam is only valid within a TEST_P function.
@ahmadsharif1 @scotts any tip on how to write this test over both swscale and filtergraph without just duplicating the code?
There was a problem hiding this comment.
I'm not experienced in GTEST, but looking through the docs, you may want to make a test fixture that accepts the color conversion library as an option: https://google.github.io/googletest/advanced.html#value-parameterized-tests
I'm not sure if you can extended the current one (on line 41), or make a new one.
There was a problem hiding this comment.
You could call getparam from the parent and pass it down here?
The other way of doing this is parameterized classes but that seems overkill to me.
There was a problem hiding this comment.
Thanks for your feedback. I tried a few things, but couldn't get it to work (new kind of error each time). I just went for duplicating the whole thing, I hope that's OK. We already have some duplicated tests in this class.
4 participants
Up until now, the
preAllocatedOutputTensorparameter of our low-level decoding functions was only considered forswscale. It is now honored in all cases, i.e. whenfiltergraphis used.