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

setTimestamp improvement for MPEG TS files #2144

Merged
merged 5 commits into from Dec 24, 2023
Merged

Conversation

anotherche
Copy link
Contributor

Changes to setTimestamp function to resolve issue #2127.
Also improved accuracy of frameNumber calculation in grabFrame function.
MPEG TS files have a couple of peculiarities: they often start with rather large timestamp values (perhaps as a result of cutting?), and key frames are very frequent. Because of this, jumps to arbitrary frames often occurred with large errors in the previous code. In the new code, avformat_seek_file is called repeatedly until a key frame earlier than the desired frame is reached.

@saudet
Copy link
Member

saudet commented Dec 13, 2023

@Aismy Please give this a try!

@saudet
Copy link
Member

saudet commented Dec 17, 2023

@anotherche Please revert unnecessary changes in formatting

@saudet
Copy link
Member

saudet commented Dec 18, 2023

I never quite understand why you change so many lines every time you touch this code, but please leave the "old quick seeking code used in JavaCV versions prior to 1.4.1" like it was

@saudet
Copy link
Member

saudet commented Dec 18, 2023

I mean, do a copy/paste of this code and just leave it as is, without touching the formatting either, unless there's a good reason to change it:
https://github.com/bytedeco/javacv/blob/1.4/src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java#L454-L488

@anotherche
Copy link
Contributor Author

I never quite understand why you change so many lines every time you touch this code, but please leave the "old quick seeking code used in JavaCV versions prior to 1.4.1" like it was

All because users want accuracy in seeking. In fact, I only change the code to accommodate these wishes. Now it was necessary to provide a cyclic call of the avformat_seek_file function. In order to avoid unnecessary code repetition, I had to explicitly separate the old (the "old quick seeking code used in JavaCV versions prior to 1.4.1") and the new seek algorithm. The "old quick seeking code used in JavaCV versions prior to 1.4.1" is still in place.

By the way, during my checks on a large number of files, the new algorithm always works the same or faster than the old one... but it works accurately (if of course the file itself is fine, but if the file is corrupted, then the old algorithm works wrong too)

I mean, do a copy/paste of this code and just leave it as is, without touching the formatting either, unless there's a good reason to change it: https://github.com/bytedeco/javacv/blob/1.4/src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java#L454-L488

I'm not sure I understand what is meant. I thought you meant that I used tabs in the code. But I had already replaced them with spaces.
Are you suggesting reverting back to the version from 1.4.1 altogether? But this code often works incorrectly.
Or am I misunderstanding what you are suggesting?

@anotherche
Copy link
Contributor Author

Oh, I apologize, I noticed that I actually forgot after a few tries to change av_seek_frame function to avformat_seek_file. I'll fix it now. Otherwise - the old code is still the same as it was before.

@anotherche
Copy link
Contributor Author

Again, I apologize. The confusion occurred after I was trying to understand why avformat_seek_file does not always do what it is supposed to do (this is what causes all the errors during seek). I tried using other ffmpeg functions for seeking and didn't notice that I left av_seek_frame in the code instead of the avformat_seek_file. Before the PR I checked that the code works in different modes (old, new). But both functions produce the same result, so I didn't notice the substitution.

@anotherche
Copy link
Contributor Author

anotherche commented Dec 18, 2023

I mean, do a copy/paste of this code and just leave it as is, without touching the formatting either, unless there's a good reason to change it: https://github.com/bytedeco/javacv/blob/1.4/src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java#L454-L488

Just in case, this part is now fully compliant with this. The faint differences you see appeared back in 1.4.2. (and then in 1.5.6), so we cannot just simply copy/paste the code from 1.4.

@anotherche
Copy link
Contributor Author

One last little change. I put the old comment (comparing to timestamp +/- 1 avoids...) back to its original place. Now there's full compliance.

@anotherche
Copy link
Contributor Author

anotherche commented Dec 18, 2023

Let me explain again in detail why the proposed changes are justified.
The problem of the setTimestamp function not working accurately enough occurred to me many times in version 1.4 and earlier, which is why I proposed the changes released in 1.4.1. This required a significant revision of the setTimestamp code.

However, not all problems were actually solved. There was still a problem originally related to the incorrect operation of ffmpeg's avformat_seek_file function. Despite the fact that this function should provide transition to the nearest preceding keyframe, sometimes it goes to the nearest next keyframe. This results in an inability to get the desired video/audio position.

In addition, there have been complaints about slower code, which is why the legacy code has returned in version 1.4.2. Since then, all versions of setTimestamp have had the option to use the legacy algorithm from 1.4 (but it also sometimes had to be modified, e.g. when switching to the new frame grabbing algorithm).

The problem with incorrect work of avformat_seek_file was decided to be fixed only before the release 1.5.5, after the issue with inaccurate seeking to audio frames (if I remember). The only way to get around the avformat_seek_file flaw was to ask to go to an even earlier timestamp first.

However, even then I realized that this approach would occasionally fail in some rare cases, and that it was necessary to redesign setTimestamp even further to provide calling avformat_seek_file in a loop, asking for increasingly earlier timestamps until the desired keyframe was obtained.

And here, in the last issue, it was shown that for MPEG TS files this happens excessively often. So this change had to be introduced. This change required the old and the new algorithm each use their own logic to call avformat_seek_file. So, it is because of this the changes look so big. But note that the legacy code is still here, and that the final version of setTimestamp uses it in the closest way possible.

@saudet
Copy link
Member

saudet commented Dec 23, 2023

I understand what you're trying to do, but refactoring the whole method every time there's a little something to fix doesn't seem like the right way to go. Anyway, if you're comfortable frequently rewriting experimental code like that, that's fine, as long as the old code remains usable just in case, I'm OK with merging this. So, are you done with this pull request? Good to merge?

@anotherche
Copy link
Contributor Author

Yeah, it's done. Merry Christmas, and may the world go back to normal!

@saudet
Copy link
Member

saudet commented Dec 24, 2023

Awesome! Thanks for the fixes, and Merry Christmas to you too! From what I understand of your research topic, you're working on stuff that could be useful to renewable energy systems? In any case, you're doing your part to "fix" the world, and the rest is out of our control...

@saudet saudet merged commit b65e7f7 into bytedeco:master Dec 24, 2023
@anotherche
Copy link
Contributor Author

From what I understand of your research topic, you're working on stuff that could be useful to renewable energy systems?

Yes, that's right, some of my research has to do with renewable energy technologies and smart materials. More precisely, they are related to the mechanisms of the processes that are used in such technologies. Programming has always been necessary for me as a tool to implement numerical solutions to research problems. So at one time I needed a java interface to opencv and ffmpeg, which I found in javacv. I am glad that I myself have been useful in some way for the development of these tools and for their application (for example, there are researchers who are happy to be able to import video into ImageJ, a program for scientific analysis of image stacks, which was implemented using javacv).

@saudet
Copy link
Member

saudet commented Dec 25, 2023

So I'm curious, what kind of software are you using for those projects? Do you still use Java a lot?

@anotherche
Copy link
Contributor Author

mostly Java, C#, C

@saudet
Copy link
Member

saudet commented Dec 25, 2023

That's interesting, I don't have the impression that a lot of science is happening in Java these days. Frameworks like Smile and Spark that have been developed at great length seem pretty much abandoned. Where's the action happening?

@anotherche
Copy link
Contributor Author

Personally, I don't use framewoks like that, just plain Java. I started using it when I needed to analyze images (time lapse shots or video of transofmations taking place in crystals) and recognize various elements in these images (geometry of transofmations, shape of crystals). The most convenient tool for me turned out to be ImageJ, written in Java and allowing me to use my own plugins. Accordingly, to create the necessary plugins, I needed a Java interface for opencv and ffmpeg, so - javacv.
C, C++, C# is used to write code to solve various differential equations and nonlinear optimization problems. These are either completely custom developments, or programs using standard computational libraries (like NAG, MathNet.Numerics, Meta.Numerics), or insertions into packages that support C-like code or external dynamic libraries.

@anotherche
Copy link
Contributor Author

By the way, about the image series. In order to uniformly combine the shooting process itself with the subsequent image analysis, I also wrote the code to control the camera in Java, using the Gphoto2 library over jna. In this regard, I had some thoughts about trying to develop an interface for Ghoto2 using javacpp. If I had the time...

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.

None yet

2 participants