-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Read video from memory newapi #6771
Conversation
This segfaults on certain videos. Here's a partial-reproduction script (the videos are stored in a pandas dataframe):
I'm working to get approval of the public copy of the data. In the meantime, the error is:
|
@vedantroy Thanks for reporting this. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd love to make the tests stricter but otherwise good to go :)
assert len(vr_pts) == len(vr_pts_mem) | ||
|
||
# compare the frames and ptss | ||
for i in range(len(vr_frames)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is like-for-like decoding (using verifiably the same backend), we should probably insist on either a max delta or completely equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually there are two videos for which this is not true: SOX5yA1l24A.mp4
I verified with this code by reading twice from file and also the result is not the same:
In [3]: reader1 = VideoReader('test/assets/videos/SOX5yA1l24A.mp4')
In [4]: reader2 = VideoReader('test/assets/videos/SOX5yA1l24A.mp4')
In [5]: reader1_frames = []
In [6]: reader2_frames = []
In [8]: for r1frame in reader1:
...: reader1_frames.append(r1frame["data"])
In [9]: for r2frame in reader2:
...: reader2_frames.append(r2frame["data"])
In [12]: for i in range(len(reader1_frames)):
...: if torch.all(reader1_frames[i].eq(reader2_frames[i])):
...: print(f"Frame {i} is not equal")
Frame 0 is not equal
Frame 2 is not equal
Frame 3 is not equal
Frame 4 is not equal
Frame 5 is not equal
Frame 6 is not equal
Frame 7 is not equal
Frame 8 is not equal
Frame 9 is not equal
Frame 10 is not equal
Frame 11 is not equal
Frame 12 is not equal
....
Do you know if for this particular format the decoding could be non deterministic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually surprising to me, is this non deterministic behaviour also occur on the stable API (if we specify same pts)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup; we use the same backend.
@jdsgomes can you check if it happens on the pyav
?
Maybe this is a cue that we should reconsider our selection of testing videos.
Specifically, I think we should re-sample them again from whatever are most used datasets these days, plus some FFMPEG fate situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that that is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please note that there is a bug in the test code (checking for equal instead of not equal) but still there are some frames that are different. This happens both in pyav and video_reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be related: https://video.stackexchange.com/questions/18902/why-ffmpeg-works-non-deterministically
In this case, I think this test is fine since it can handle the non-deterministic behaviour.
@@ -165,7 +165,7 @@ struct MediaFormat { | |||
struct DecoderParameters { | |||
// local file, remote file, http url, rtmp stream uri, etc. anything that | |||
// ffmpeg can recognize | |||
std::string uri; | |||
std::string uri{std::string()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
(don't know C++ well, so I'm just curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to initialise the uri value as empty string
This reverts commit 6c92e94.
test/test_videoapi.py
Outdated
@@ -76,8 +76,14 @@ def test_frame_reading(self, test_video): | |||
|
|||
# compare the frames and ptss | |||
for i in range(len(vr_frames)): | |||
assert av_pts[i] == vr_pts[i] | |||
torch.test.assert_equal(av_frames[i], vr_frames[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in the wrong section so reverted the commit. My comment regarding why we cannot make the tests strict from read_from_memory still holds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jdsgomes , I think overall the PR looks good. I will approve first to unblock, but lets wait for approval from @bjuncek before merging.
Also for vedantroy comment, I can't really reproduce it on my side. In my opinion, we can merge first but open an issue for the possible problem on segfault so we can come back to it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock, and starting a new issue to track video assets.
Hey @jdsgomes! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Summary: * add tensor as optional param * add init from memory * fix bug * fix bug * first working version * apply formatting and add tests * simplify tests * fix tests * fix wrong variable name * add path as optional parameter * add src as optional * address pr comments * Fix warning messages * address pr comments * make tests stricter * Revert "make tests stricter" This reverts commit 6c92e94. Reviewed By: YosuaMichael Differential Revision: D40588171 fbshipit-source-id: 1c55b5ebd0bfdd3308931d218269a19481eb3ca9
adds read from memory functionality in the new video API.
Addresses #6603