Skip to content
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

Set should_buffer to True by default in _read_from_stream video.py #2201

Merged
merged 1 commit into from
May 14, 2020

Conversation

mjunyent
Copy link
Contributor

@mjunyent mjunyent commented May 11, 2020

Following the discussion, fixes #1884

I used VideoClips and the following code to test the drop in performance on _read_from_stream:

vc = VideoClips(["video.mp4"])
s = time.time()
for i in tqdm(range(vc.num_clips())):
    _ = vc.get_clip(i)
print(f"Get {vc.num_clips()} clips: {time.time()-s}")

In my local machine, with a 2:30s video (3577 clips) and after several iterations the mean times where 997s for the current code and 1050s for the version with should_buffer enabled. This accounts for an increase of ~0,015s per clip.

The default VideoClips length is 16 frames and the buffer size is 5 frames on each side. The drop in performance should be less noticeable with a bigger length.

The code related to DivX might still disable the should_buffer if it's not necessary on DivX cases:

if stream.type == "video":
# DivX-style packed B-frames can have out-of-order pts (2 frames in a single pkt)
# so need to buffer some extra frames to sort everything
# properly
extradata = stream.codec_context.extradata
# overly complicated way of finding if `divx_packed` is set, following
# https://github.com/FFmpeg/FFmpeg/commit/d5a21172283572af587b3d939eba0091484d3263
if extradata and b"DivX" in extradata:
# can't use regex directly because of some weird characters sometimes...
pos = extradata.find(b"DivX")
d = extradata[pos:]
o = re.search(br"DivX(\d+)Build(\d+)(\w)", d)
if o is None:
o = re.search(br"DivX(\d+)b(\d+)(\w)", d)
if o is not None:
should_buffer = o.group(3) == b"p"

I don't know if that part should be removed or if it's better to keep it.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think a 5% speed penalty is acceptable in this case until we figure out a better way.

cc @stephenyan1231 for awareness.

@fmassa fmassa merged commit cc4c2af into pytorch:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VideoClips Assertion Error
2 participants