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

Add seeking #14

Merged
merged 8 commits into from
Oct 24, 2024
Merged

Conversation

kevinschweikert
Copy link
Contributor

Hey, I saw this in you TODO issue #1 and could need it myself.
I've never written any C code, so I probably made some mistakes, but I hope this will help with some improvements.

I added a function to seek to a time in seconds, which will seek the reader.
Additionally, I've added the frame rate to the Reader struct, so the user can use this information for further calculations

lib/reader.ex Outdated Show resolved Hide resolved

avcodec_flush_buffers(reader->c);

if (av_seek_frame(reader->fmt_ctx, reader->stream_idx, frmseekPos, AVSEEK_FLAG_BACKWARD) < 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.

In my test video, there is a keyframe every second with a framerate of 25 fps. But when i seek, i can only get to frames which are a multiple of 250 frames...couldn't figure out where the issue is. Maybe we have to explicitly calculate P frames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was indeed the issue! av_seek_frame does only seek to an I frame and you have to send packets to the decoder until you reach the desired timestamp. After some checking i confirmed that my test file had a keyframe every 10 seconds instead of every 1 seconds

Copy link
Member

Choose a reason for hiding this comment

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

Nice, I am impressed!

c_src/xav/xav_reader.c Outdated Show resolved Hide resolved
test/reader_test.exs Outdated Show resolved Hide resolved
lib/reader.ex Outdated Show resolved Hide resolved
lib/reader.ex Outdated Show resolved Hide resolved
lib/reader.ex Outdated Show resolved Hide resolved
c_src/xav/xav_reader.c Outdated Show resolved Hide resolved
Comment on lines 188 to 192
int64_t frmseekPos = av_rescale_q(
(int64_t)(time_in_seconds * AV_TIME_BASE),
AV_TIME_BASE_Q,
reader->fmt_ctx->streams[reader->stream_idx]->time_base
);
Copy link
Member

Choose a reason for hiding this comment

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

I think that what we need to do here is to convert time_in_seconds to frame number. Given that time_base is 1/fps we should do time_in_seconds * time_base.den / time_base.num so :

Suggested change
int64_t frmseekPos = av_rescale_q(
(int64_t)(time_in_seconds * AV_TIME_BASE),
AV_TIME_BASE_Q,
reader->fmt_ctx->streams[reader->stream_idx]->time_base
);
av_rescale(time_in_seconds, time_base.den, time_base.num);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not exactly..av_seek_frame expects the seeking position in the same timebase as the selected stream not the frame number. I did some further research and it seems, that this is the correct way to calculate the seeking position and preserving the float accuracy. For example the file in test/fixtures/sample_h264.mp4 has a timebase of 1/15360 and AV_TIME_BASE is 1000000

Copy link
Member

Choose a reason for hiding this comment

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

Ohh okay 🤦‍♂️ Could you please add a comment or link some resource that explains what we do here? I mean, what's the difference between AV_TIME_BASE and AVStream.time_base and why we need to use both in this calculation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely!

@kevinschweikert
Copy link
Contributor Author

@mickel8 thank you very much for your feedback! I finally figured it out how to seek to the correct timestamp. See my comment. I've addressed almost all the comments and we just have to agree on the time_base calculation 😄

@mickel8
Copy link
Member

mickel8 commented Oct 24, 2024

Perfect, thanks a lot! If we could just add a comment explaining what happens in timestamp calculation, we are ready to merge!

@mickel8 mickel8 merged commit 431afd8 into elixir-webrtc:master Oct 24, 2024
@mickel8
Copy link
Member

mickel8 commented Oct 24, 2024

Merged, thank you!

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.

2 participants