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

Remove unnecessary optionality #271

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Oct 16, 2024

Because we set this value to 0 initially, it is always there. Since it's always there, there's no reason for it to be an optional. However, 0 is not a great default, since streams can technically have pts values less than 0. So let's just set it to be INT64_MIN, which is what we thought was appropriate if the value was not set (even though it always was!).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 16, 2024
@scotts
Copy link
Contributor Author

scotts commented Oct 16, 2024

Benchmarks show this change is performance neutral. Three runs from before:

                                             |  10 seek()+next()  |  1 next()  |  10 next()  |  create()+next()
      TorchcodecCompiled                     |        48.9        |     9.8    |     13.4    |                 
      TorchcodecNonCompiled:                 |        48.9        |     9.6    |     12.9    |                 
      TorchcodecNonCompiled:num_threads=1    |       112.1        |     7.0    |     11.9    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.6   

      TorchcodecCompiled                     |        49.0        |     9.9    |     13.6    |                 
      TorchcodecNonCompiled:                 |        48.5        |     9.7    |     12.9    |                 
      TorchcodecNonCompiled:num_threads=1    |       113.1        |     7.0    |     12.0    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.7    

      TorchcodecCompiled                     |        49.1        |     9.8    |     13.5    |                 
      TorchcodecNonCompiled:                 |        49.2        |     9.6    |     14.4    |                 
      TorchcodecNonCompiled:num_threads=1    |       115.9        |     7.0    |     14.3    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.6            

After:

                                             |  10 seek()+next()  |  1 next()  |  10 next()  |  create()+next()
      TorchcodecCompiled                     |        48.7        |     9.8    |     13.5    |                 
      TorchcodecNonCompiled:                 |        49.1        |     9.5    |     14.7    |                 
      TorchcodecNonCompiled:num_threads=1    |       112.0        |     6.9    |     12.0    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.7      

      TorchcodecCompiled                     |        49.2        |     9.7    |     13.3    |                 
      TorchcodecNonCompiled:                 |        49.4        |     9.7    |     14.6    |                 
      TorchcodecNonCompiled:num_threads=1    |       112.0        |     7.0    |     12.1    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.6    

      TorchcodecCompiled                     |        49.0        |     9.7    |     13.4    |                 
      TorchcodecNonCompiled:                 |        48.2        |     9.6    |     12.8    |                 
      TorchcodecNonCompiled:num_threads=1    |       112.5        |     6.9    |     12.1    |                 
      TorchcodecNonCompiled                  |                    |            |             |        9.7      

@scotts scotts marked this pull request as ready for review October 16, 2024 16:02
@@ -308,7 +308,7 @@ class VideoDecoder {
// The desired position of the cursor in the stream. We send frames >=
// this pts to the user when they request a frame.
// We set this field if the user requested a seek.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment stale now?

Alternative change would be to not initialize this to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. We can make it accurate with a one-word change.

Not initializing this variable to be 0 would actually have the same behavior as my change, although my change is simpler code. That is, we would use INT64_MIN until a seek happens in both cases.

@scotts scotts merged commit c6a0a5a into pytorch:main Oct 16, 2024
22 checks passed
@scotts scotts deleted the discard_frames branch October 16, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants