Update resource generation script to python #828
Conversation
meta-cla
bot
added
the
CLA Signed
There was a problem hiding this comment.
Thanks @Dan-Flores , I left a few comments below that should be easy to address, so I'll approve to unblock.
QQ regarding
In testing, I observed that the generated reference resources differed slightly from the currently checked-in files, but the resulting tensors were the same between the shell and python script, so its likely a difference in FFmpeg.
Can you clarify how you observed that the generated references resources were different? I'm a little confused because I would assume that in order to compare the reference resources, we'd be looking at the resulting tensors (which, as you noted, appear to be equal)
test/generate_reference_resources.py
Outdated
|
|
||
|
|
||
| def convert_image_to_tensor(image_path): | ||
| if not os.path.exists(image_path): |
There was a problem hiding this comment.
Here and everywhere else in this file, let's try to modernize the codebase and rely on pathlib and pathlib.Path functionalities, instead of os.*.
There was a problem hiding this comment.
The comment above about using pathlib still stands, but I wonder whether the "if file exists" check makes sense? Shouldn't we expect the file to exist, and error if not?
There was a problem hiding this comment.
There was one issue I encountered in the for loop over these arrays:
STREAMS = [0, 3]
FRAMES = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 15, 20, 25, 30, 35, 386, 387, 388, 389]
One stream index does not have as many frames, hence why stream 3's checked in frames go up to nasa_13013.mp4.stream3.frame000389.pt, but stream 0's checked in frames end at nasa_13013.mp4.stream0.frame000035.pt.
In these cases, FFmpeg errors quietly so the script does not create or delete the frames:
[vost#0:0/bmp @ 0x145f23cb0] No filtered frames for output stream, trying to initialize anyway.
...
[out#0/image2 @ 0x145f23150] Output file is empty, nothing was encoded(check -ss / -t / -frames parameters if used)
test/generate_reference_resources.py
Outdated
| print(img_tensor.shape) | ||
| print(img_tensor.dtype) |
There was a problem hiding this comment.
I noticed they were present in the previous util, but they were probably debug leftovers that we can now remove?
test/generate_reference_resources.py
Outdated
| "-q:v", | ||
| "2", | ||
| output_bmp, | ||
| ] |
There was a problem hiding this comment.
I haven't looked at the cmd definition in details, but maybe there are opportunities to factorize some of the calls?
test/generate_reference_resources.py
Outdated
| # All rights reserved. | ||
| # | ||
| # This source code is licensed under the BSD-style license found in the | ||
| # LICENSE file in the root directory of this source tree. |
There was a problem hiding this comment.
I think this copyright notice should be at the very top - although that might conflict with the #! line, so we should make sure it can still be invoked the intended way.
I apologize for the confusing language @NicolasHug, when I mentioned generated references resources, I was referring to the tensors. |
This PR updates the resource generation script to be in python and integrates the
convert_image_to_tensor.pyscript, as discussed in #820.test/utils.py, similar to the other TestVideo/Audio definitions.Note:
In testing, I observed that the generated reference resources differed slightly from the currently checked-in files, but the resulting tensors were the same between the shell and python script, so its likely a difference in FFmpeg.