Fix: PyAV does not support floating point numbers with decimals as FPS when writing and will throw in case this constraint is not satisfied. #2334
Conversation
…S when writing and will throw in case this constraint is not satisfied.
QuentinDuval
changed the title
| # PyAV does not support floating point numbers with decimal point | ||
| # and will throw OverflowException in case this is not the case | ||
| if isinstance(fps, float): | ||
| fps = np.round(fps) |
There was a problem hiding this comment.
nit: we would prefer to use native python functions over numpy functions (in this case round from python).
The reason being that numpy can't be torchscripted, while many python builtins can.
In here this doesn't make a difference because this function can't be torchscripted anyway (due to pyav), but this is something good to keep in mind.
There was a problem hiding this comment.
Hi, thank you for the comment ! This makes totally sense: I can replace this with round.
Regarding the fix itself: the issue is more a problem of symmetry:
- reading with read_video will return me a floating point number
- writing with this floating point number will not work (and the error is not clear)
The net effect is that if one tries to read a video to write it afterwards (for instance, to resize it), the error will be very hard to understand. My proposal is not necessarily the best way to fix this though: we could at least add a warning. Or we could decide to try something more ambitious.
There was a problem hiding this comment.
That's a valid point.
The original reason why I added write_video was purely for testing purposes (to validate that video reading was working), so I didn't really think much about it (maybe I should have made it private?). But looks like more people are relying on it / using it.
The decision between using ceil / floor / round is not yet very clear to me yet. round seems ok, so let's go for it. But given that videos can have a varying fps, the concept of a single fps is not really well-defined.
…S when writing and will throw in case this constraint is not satisfied.