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

Improve setting framenumber #1851

Merged
merged 56 commits into from
Aug 2, 2022
Merged

Improve setting framenumber #1851

merged 56 commits into from
Aug 2, 2022

Conversation

anotherche
Copy link
Contributor

Minor changes to the calculation of frameNumber to improve the accuracy of the methods setFrameNumber, setVideoFrameNumber, setAudioFrameNumber. Solves #1849

anotherche and others added 30 commits February 11, 2018 12:29
setting frameGrabbed to false after grabbing; 
update of videoStream and audioStream variables after starting grabber; 
add helper API functions hasVideo(), hasAudio() and
printFormatInfo(int);
Add test seeking in audio only and video only files;
add getLengthInVideoFrames() alias of getLengthInFrames();
cleanup;
@anotherche
Copy link
Contributor Author

I don't know if I did it right. It looks like I'm completely confused in all these operations. I apologize...

@anotherche
Copy link
Contributor Author

The only change I mean is e980ac2

@saudet
Copy link
Member

saudet commented Jul 28, 2022

The only change I mean is e980ac2

That sounds like pretty much the same thing as Math.round(). How is it different from what was there before pull #1734 (review) ?

@anotherche
Copy link
Contributor Author

The only change I mean is e980ac2

That sounds like pretty much the same thing as Math.round(). How is it different from what was there before pull #1734 (review) ?

Absolutely right! Because floor(x+0.5) is equivalent for round(x). Moreover, we can revert to Math.round in all three methods of framenumber setting (however the addition of 500000L should be done anyway).

@saudet
Copy link
Member

saudet commented Jul 28, 2022

Could you fix the branch somehow so that we can see only your changes?

@saudet
Copy link
Member

saudet commented Jul 28, 2022

It doesn't look like your tests are liking these changes...

@anotherche
Copy link
Contributor Author

anotherche commented Jul 29, 2022

It was because of copy-paste mistake in setAudioFrameNumber. It is fixed now.
Additionally, I removed one unnecessary part of test (not the part that caused the failure!). I removed this part because it was not about checking setTimeStamp code, but about checking that adjacent audio and video frames in the file differ by no more than 1 second. This has nothing to do with setTimeStamp, but with how ffmpeg fills a container with different types of streams.

@saudet
Copy link
Member

saudet commented Jul 29, 2022

And you're sure this doesn't cause a regression for issue #1697?

@anotherche
Copy link
Contributor Author

And you're sure this doesn't cause a regression for issue #1697?

I believe it doesn't due to timestamp shift by 500000/frameRate in setVideoFrameNumber. But to be sure I will add short explicit test for the issue #1697.

@anotherche
Copy link
Contributor Author

It passes :)

@saudet saudet merged commit bc1ea9d into bytedeco:master Aug 2, 2022
@anotherche anotherche deleted the framenumber branch August 2, 2022 06:57
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.

4 participants