Skip to content

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
int64_t currentDuration = 0;
// 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 meta-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