-
Notifications
You must be signed in to change notification settings - Fork 277
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
Fix invalid starting PTS value (blank 1st frame) #695
Conversation
…humbnailing and video players.
Codecov Report
@@ Coverage Diff @@
## develop #695 +/- ##
========================================
Coverage 50.41% 50.42%
========================================
Files 155 155
Lines 13315 13297 -18
========================================
- Hits 6713 6705 -8
+ Misses 6602 6592 -10
Continue to review full report at Codecov.
|
So from testing with ffprobe, I found that video files are now starting at time:0.00000; but audio only exports are still starting a fraction of a second after 0. |
The change I made to And I verified that the audio file I was using had an original start time of 0. |
src/FFmpegWriter.cpp
Outdated
if (pkt.dts != AV_NOPTS_VALUE) | ||
pkt.dts = av_rescale_q(pkt.dts, audio_codec_ctx->time_base, audio_st->time_base); | ||
if (pkt.duration > 0) | ||
pkt.duration = av_rescale_q(pkt.duration, audio_codec_ctx->time_base, audio_st->time_base); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a pkt.duration
, seems like that could be used to increment write_video_count
. As the documentation says, AVPacket::duration
"Equals next_pts - this_pts in presentation order." (Also, pretty sure av_packet_rescale_ts()
is a better choice than av_rescale_q()
here, too.)
…escale_ts() method. I'm just not sure the backwards compatibility of this approach with older FFmpeg versions.
I'm curious if the switch to av_packet_rescale_ts() will cause builder failures on older versions of FFmpeg. Testing this now. |
It's existed since at least FFmpeg 2.4, possibly earlier. (That's as far back as I checked.) |
…mp variables to something more sane.
I just pushed a commit that adds For the record, for the Webm unit test, this is the why:
But equally worrying is the output that comes before it, while the test is running: Folded because it's frickin' Y000000GE...
|
P.S> It's also always possible to configure CMake with Doing so reveals that the next test — test 54, "FFmpegWriter:Options_Overloads" — also shows strange output, though it passes (because it's not testing the output, only the API):
...But those are the only two. |
Here are the results of ffprobe-mp4.log They both show the same basic thing:
...and so on for every frame except the first. |
… some codecs (such as vp8), this approach breaks due to differences in the timebase vs the framerate. For example, if the timebase is an inverse of the FPS, everything works. But if the timebase is not, for example 1/1000000, this approach breaks.
Thanks @ferdnyc, however I could not get any output from CTest without the following: I'm probably missing something simple, but I got your changes/commit, cleared the CMake temp files, and re-ran cmake (with and without the |
I ended up reverting a couple uses of the |
Sorry, that's my fault, I was unclear.
There's both the general So both |
To get complete test output, with the |
(On the plus side, by default the |
@ferdnyc On a side note, I've been playing more with Sentry.io, and added/assigned a few strange ones to your user, if you are curious or interested. 👍 Thx! |
@jonoomph Sure, I'll take a look, thanks! |
Great to see this fixed, will give it a try soon. Thanks guys. |
Yeah, agreed. What I found is that it does increment — but only when the frame sent results in the creation of an output AVFrame. Buffered frames and partial frames (for audio) don't create new frames, so the packets have no duration. That especially happens in the beginning of encoding, because the encoder buffers a bunch of frames for predictive motion estimation before it starts sending back packets. When looping through with some debug
from the video encoder. (I guess that fits with the documentation line about duration being equal to Going through the calls like that did reveal another issue, though. I now know where all of those "ignoring attempt to flush an encoder..." messages on stderr are coming from. (To be continued...) |
Our current encoding code is still too locked into the old idea of "Send a frame, receive a packet." And it proceeds based on that assumption. But encoding isn't that strictly ordered, there isn't a 1:1 relationship between frames sent and packets received. IOW, we're not following the official API guide for decoding/encoding:
In So what we're doing (in the FFmpeg 3.2+ branches) is: ret = avcodec_send_frame();
while (ret >= 0) {
ret = avcodec_receive_packet();
if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF) {
avcodec_flush_buffers();
break;
}
if (ret == 0) {
got_packet = 1;
break;
}
if (got_packet) {
av_interleaved_write_frame();
} and that's one call to that function. No more receives will be attempted until the next call. What we need to be doing, instead, is something more like: ret = avcodec_send_frame();
while (ret >= 0) {
ret = avcodec_receive_packet();
if (ret < 0) {
if (ret != AVERROR(EAGAIN)) {
// error-handling code
}
break;
}
av_interleaved_write_frame();
} So that, if no packet is received, that's OK and will be ignored — it won't trigger an unnecessary flush attempt. And if more than one packet is waiting on the interface, we'll retrieve and write them all to the file immediately. (I also get the impression that the receive loop will receive both audio and video data, regardless which one was last sent, and probably should be done separate from the send-audio/send-video code.) |
Another possible reason the
(I say "may be" because it's not 100% clear to me whether "the data" in that statement means just the packet data buffer ( |
Nope! That's wrong, because it |
Fix invalid starting PTS value, preventing blank 1st frames on some thumbnailing and video players. Turns out, we were incrementing the starting timestamp before writing a packet at position 0. I did some tests, and now ffprobe reports a start time of 0.0000000, instead of 0.033008.