Details

Reviewers

Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)
Richard Antalik (ISS)

Summary

FFMPEG frame seek bug fix

re: Issue T72347

Blender was using two slightly different strategies to load a video frame depending on if we were advancing to nearby frames or seeking from far away.

Fix:

Make scanning work the same as forward stepping (by looking ahead).
Notify the user if we detect a frame synchronization problem. Try to offer a helpful message.

Replicating:

-see T72347

Investigate the problem using a breakpoint:
Checkout 39e37c9db56 "Consistency check for frame scanned pts" in this diff.
Set a breakpoint in source/blender/imbuf/intern/anim_movie.c at the line containing "av_log(anim->pFormatCtx, AV_LOG_ERROR, "PTS for frame differs from previous decode" which should complain on frame 60 in the last step above. If not check preferences->system->sequencer cache limit and make sure it's still 1 !!

I expect to do more testing, specifically related to how it may help ( or not ) audio sync in video editing and work with previous saved blender projects ( including those with video proxies ). Any help here would be appreciated ( recommend users to test, supply files to test etc. ).

Background:

Spoiler: What it all boils down to is that we can only rely on the decoded pts timestamps.

When decoding compressed video frames we have very few "knowns" unless we want to get into working with specific compression codecs individually. We are "flying by wire" to allow the codecs the freedom to prioritize compression/speed/access as they see fit.

What we don't know:

The packets before decoding are not necessarily in the same order as the decoded frames coming out.
The presentation timestamp ( PTS) on the encoded packets is not necessarily accurate or present. We can only trust the PTS from the decoded frame ( and remember it does not correlate with the order of the compressed packets going in ).

So all we can do reliably is search forward from an iframe to find a desired pts. This was already being done.
Some timeframes might have 0,1 or multiple images associated with them, therefore the only way to know if we have the proper image is to decode until we decode a PTS meant for the next timeframe.

Strategy:

As currently done, we need to search forward from the nearest i-frame to find the frame we are looking for.
As currently done, we decode as we search forward in order to get an accurate pts.

New: Look ahead. If we search forward and stop when we find a pts for the next frame.

Compatibility with current Proxy timecode files:

The frame finding is done so that the pts_to_search value is an inclusive top end so that exact values will match the current frame ( and thus we can use pts values from the existing indexing methods - and should load old indexes properly ).

Code changes ( all within source/blender/imbuf/intern/anim_movie.c and IMB_movie.h ):

Main goal: We needed to look at the pts for the next frame during scanning in order to find wanted pts. Before this we were only doing this when stepping forward a frame at a time ( and in some cases assuming there was a no missing frame ).

Change 1) Add in a way to verify we were looking for the same pts as the last time this frame was shown: use arrayFrame2PTS[] to keep track of former search pts results.
Change 2) Search for PTS based on timecode calculation only.
Change 3) "Double buffer" ffmpeg_decode_video_frame() so we can look forward at the next pts within ffmpeg_decode_video_frame() for all situations instead of only in forward step searches occuring in ffmpeg_fetchibuf()
Change 4) Use ffmpeg_decode_video_frame_scan() in the aforementioned forward step search to catch any skipped frames.
Change 5) Empty double buffer at EOF ( now a bit more complicated, now have anim->pCurrFrameComplete and anim->pNextFrameComplete )
Change 6) reinitialize arrayFrame2PTS ( and honour numframes from proxied indexing ) if user changes indexing method ( but leave any tracking already done, for better or worse. Handle this better?? ).

Notes:

I originally started coding a fast container dts based index to map video frames to blender frames ( focused on motion matching: ignore timing until later ) .. but decided the way forward was to use the timestamps instead of optimizing on frames. Utilizing every frame independent of timing can still be done with a proxy index although this will lead to audio synchronization issues.

I commented out a line in indexer.c to allow me to build a tc proxy without image proxies during testing. I've left the change in for now but it's not necessary as part of this bug fix. Should this change be left in?

This patch keeps two decode buffers available and uses FFMPEG's reference counting to let the codec decide if it needs to copy the buffer or not. This may affect performance slightly depending on the codec implementation. From my experience this is not an issue.

H264 SPS/PPS issue - iframes are not necessarily independent! H264 AnnexB is the solution.

Diff Detail

Repository

rB Blender

Branch

ffmpeg_pts (branched from master)

Build Status

Buildable 6196
Build 6196: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Sergey Sharybin (sergey) added inline comments.Dec 17 2019, 4:44 PM

source/blender/imbuf/intern/anim_movie.c
494	use `static` qualifier.
502	Same as above.
source/blender/imbuf/intern/indexer.c
952	`context->re_use_packet = false;`

wow-- did I write that!!? ;) - thanks for catching that Sergey.

Interesting tidbit...
Reminded me of John Carmack's warnings about copy-n-paste errors..
https://www.youtube.com/watch?v=4zgYG-_ha28#t=1h01m10s
( transcript: https://www.shamusyoung.com/twentysidedtale/?p=12574) at 1:10.

Harbormaster completed remote builds in B6046: Diff 20365.Dec 18 2019, 7:27 AM

Justin Jones (jjones780) marked 7 inline comments as done.Dec 18 2019, 7:36 AM

improved variable names, plus changed _all_ camelcase names to underscored versions.

Harbormaster completed remote builds in B6047: Diff 20366.Dec 18 2019, 8:38 AM

I've downloaded the Blender ffmpeg test videos ( and all the other test via make test ).

This patch uses the fps calc whereas the previous version prioritized a frame count (if available and realistic).

This may or may not explain discrepancies. I'm looking into it further.

Notes:
One can still use a pure frame count by building a proxy with appropriate tc index.
Building a proxy index results in a different frame lengths depending on the timecode in use.

I can't seem to run the actual ctest -R ffmpeg but don't really need to for the time being. If it's a known issue please let me know.

blender/tests/python/modules/test_utils.py", line 59
    blender: pathlib.Path = None
           ^
SyntaxError: invalid syntax

I can't seem to run the actual ctest -R ffmpeg but don't really need to for the time being. If it's a known issue please let me know.
blender/tests/python/modules/test_utils.py", line 59
    blender: pathlib.Path = None
           ^
SyntaxError: invalid syntax

This indicates that you're using too old a Python version. Blender requires Python 3.7 or newer.

fix length calculation and tune pts drift allotment.

Now passes tests. I was indeed calculating length wrong by one.

More testing I want to do yet and potential changes/additions to indexing.

Harbormaster completed remote builds in B6070: Diff 20405.Dec 19 2019, 3:06 AM

It turns out the video proxy time indexing code in Blender ( indexer.c ) is unfinished(?). All the index files produced are identical despite the differing types.

Without looking at the code you can check this by looking in the BL_PROXY/video_file  directory next to the video file after producing a proxy ( and index).
>>   md5sum  record_run.blen_tc free_run.blen_tc interp_free_run.blen_tc record_run_no_gaps.blen_tc will indicate identical files.

Am I right in that the current indexing is a placeholder and is unfinished? Is this currently being worked on?
EDIT: As per the comment in IMB_imbuf.h:292:

IMB_TC_NONE = 0,
  /** use images in the order as they are recorded
   * (currently, this is the only one implemented
   * and is a sane default) */

I have some ideas for re-working the indexing in a slightly different way. Maybe best as a separate patch.

Manage double decode buffers in own function.
Still some complex intermingling.. hoping to clean it up further and/or document.

Harbormaster completed remote builds in B6172: Diff 20643.Jan 6 2020, 1:25 PM

Notes to self/FYI:

Just noticed a comment in blenlib/intern/timecode.c:71

/* hours */
/* XXX should we only display a single digit for hours since clips are
 *     VERY UNLIKELY to be more than 1-2 hours max? However, that would
 *     go against conventions...
 */

BUT, if the video uses free run timecode then it is representative of the time of day the video was recorded so could easily be at 22 hours etc. even for a 2 second clip.
Currently not an issue as Blender calculates its own timecode starting at 0;
but Blender could/should be upgraded to use timecodes specified by the user or from a source video to accommodate editing of video from synchronized cameras.

Index Timecode Options
The timecode settings for indexing don't really make sense - these are specific to the video file and since Blender doesn't read timecodes I don't see how they could specify useful indices

The source video would be using one of these timecode types
Free run - timestamp is based on wall clock
Record Run - timestamp is based on elapsed time ( but continued from last recording time )
and also ( representing IMB_TC_INTERPOLATED_REC_DATE_FREE_RUN, IMB_TC_RECORD_RUN_NO_GAPS ?)
Drop Frame - for NTSC video at 29.97fps - skip some timecode frame number assignments to stay in sync with actual time ( since 29.97fps introduces drift ).
Non-Drop Framecode - fall behind 3.6 seconds for every hour on a real clock.

Blender currently doesn't read the video's timecode - it would be in a separate data stream and also may even be unique to the recording device.

As for time-frame indexing options would be more along the lines of:
use video timing ( default)
ignore timing - use every frame
use blender timing settings
checkbox options: smart mode - pick offset with least skipped/doubled frames.

Ideally, in all of the above the video frame timing information would be stored and made accessible for sub-frame timing use and/or to recreate actual timestamps for exported video.

This article was quite informative: https://blog.frame.io/2017/07/17/timecode-and-frame-rates/

Caught some bugs ( from latest changes but also one found during simulating re-use of packets )
Modified some AV_LOG_DEBUG messages to be more descriptive.
No longer uses last found pts in repeated search... impedes debugging and otherwise not necessary.

Everything seems to work well but I will continue to test as time permits and maybe look into more re-architecting. A fresh set of eyes might offer some advice in this regard.

self-note:
Videos appear on frame 0 despite not starting until frame 1. I believe this was an existing behavior... but should be changed?

Harbormaster completed remote builds in B6196: Diff 20685.Jan 8 2020, 12:30 PM

It turns out the video proxy time indexing code in Blender ( indexer.c ) is unfinished(?). All the index files produced are identical despite the differing types.

I vaguely recall that some of the timecodes are not implemented.
But Free Run No Gaps should actually be different from others. It basically "converts" video to image sequence by "extracting" all decodable frames from the stream.

Index Timecode Options

Finally it all makes sense :) This information should be added to Manual. Do you feel like looking into it?

One thing which always puzzles me is do artists really need to know which timecode to use to have reliable motion tracking? Is there a common timecode which will "just work"? Is it possible to guess it from video itself somehow?

Videos appear on frame 0 despite not starting until frame 1. I believe this was an existing behavior... but should be changed?

This will be good to address, but as a separate patchset. Makes it easier to review and allows to more faster accept to master.

improved variable names, plus changed _all_ camelcase names to underscored versions.

That's cool. Usually we prefer such rename happen as separate change, since that is no-functional-changes which is easier to review, accept imemdiatelly and makes a function changes patch easier to understand.

I can only do basic tests with videos i've got. Is there anything you want to address still or do you think all the changes you've planned have been implemented in this patch?

One thing which would help a lot is to have documentation somewhere describing how indexing is working from a bigger picture. What is the PTS and such. All those things which are not so intuitive for non-heavy-duty-video-library-developers.
Should be careful though, this is a legacy area which requires a bigger refresh, which shouldn't really be in a way of adding new functionality.

@Sybren A. Stüvel (sybren), can you be second pair of eyes here?

source/blender/imbuf/intern/IMB_anim.h
88	In theory this should be `FramePtsRecord` since types in Blender are CamelCase. This is something what `anim` doesn't follow here, but i think we should use proper style for new code and change old one eventually to comply to new style.
89–90	Usually i see terminology like `requested` and `actual`.
147	`re_use` -> `reuse`, is one word as far as i can tell. Mind adding a comment what exactly it means?
150	This is more like `current_timecode`. `watch` doesn't really fir terminology in Blender.
source/blender/imbuf/intern/anim_movie.c
506	Can simplify to `sizeof(frame_pts_record)` (as in, remove `struct`).
877	What is double decode buffer?
879	`aanim` -> `anim`
887	Comments in blender are `/**/`. Also comments should be focused on why stuff is happening, not what is happening. So why do we need to swap frames on a new frame? Maybe it comes to a fact that we store current and next frames, but need for that is also rather mysterious.
888–890	Think you can replace it with `SWAP(AVFrame *, anim->curr_frame, anim->next_frame)`
896	Usually we do `if (some_call() == 0)`

It's great to see someone finally putting some sense in this part of Blender :)

In D6392#153490, @Justin Jones (jjones780) wrote:

BUT, if the video uses free run timecode then it is representative of the time of day the video was recorded so could easily be at 22 hours etc. even for a 2 second clip.
Currently not an issue as Blender calculates its own timecode starting at 0;
but Blender could/should be upgraded to use timecodes specified by the user or from a source video to accommodate editing of video from synchronized cameras.

👍

Index Timecode Options
The timecode settings for indexing don't really make sense - these are specific to the video file and since Blender doesn't read timecodes I don't see how they could specify useful indices

I'm glad I'm not the only one who can't make heads or tails of these options.

As for time-frame indexing options would be more along the lines of:
use video timing ( default)
ignore timing - use every frame
use blender timing settings
checkbox options: smart mode - pick offset with least skipped/doubled frames.

Ideally, in all of the above the video frame timing information would be stored and made accessible for sub-frame timing use and/or to recreate actual timestamps for exported video.

Is this something you want to address in this patch as well? Or leave it for a later one? (I would prefer the latter, unless it cannot be helped).

I agree with @Sergey Sharybin (sergey) on the re_use_packet → reuse_packet, and on the suggested split between cleanup and functional changes.

source/blender/imbuf/intern/indexer.c
606	Maybe add a comment that explains why there is a loop here.
611	Why is this a loop-breaking error, while below a similar error is not loop-breaking?
630	I quite dislike just calling something `ret` because it's returned at some point. Early returns are good, so let's keep them around. If a variable like this should really still be used, and early returns really aren't possible, rename the variable so that it has a sensible meaning, something like`last_frame_written_ok`, and include a comment why it is okay to ignore all errors except on the last-written frame. The return value seems to be a boolean, so now that things are getting cleaned up anyway, let's change that to a boolean instead of an int, and use `true` or `false` as values.
902	A comment here that explains what the function is for would be nice. In my mind "rebuild ffmpeg" means "recompile ffmpeg", and that's definitely not what's happening here ;-)
944	What is `res`? Result? Resolution? ResurrectTheAlmightyCh'thulu?
946–960	I'm not a big fan of a conditional that changes a variable that's used in the next conditional, where the value in the next conditional may or may not have been influenced by the first. You can rewrite to this: context->re_use_packet = (res == AVERROR(EAGAIN)); if (res == 0 \|\| context->re_use_packet) { frame_finished = (avcodec_receive_frame(context->codec_ctx, in_frame) == 0); } else { frame_finished = 0; // no new frame to decode } The condition `(res == 0 \|\| context->re_use_packet)` makes sense to me, because you need to receive a frame if either the previous packet send was OK or the packet needs to be reused.

Sybren A. Stüvel (sybren) requested changes to this revision.Jan 16 2020, 6:41 PM

This revision now requires changes to proceed.Jan 16 2020, 6:41 PM

Sergey and Sybren,

Thank you for having a look at this code. I know you are keeping busy with core Blender code and it's nice to get a visit!

I'll try to address your questions, and apart from my first answer, in order:

Sergey:

Q: I can only do basic tests with videos i've got. Is there anything you want to address still or do you think all the changes you've planned have been implemented in this patch?

I do have a few small changes that make sense to fit into this patch - I'm still running through some tests. I'll submit them with the requested changes ( as they dont' overlap ).

C: I vaguely recall that some of the timecodes are not implemented.

Yes, there are only TWO timecodes currently implemented. The default time/frame and the gapless.

Q: Add to manual

I'm planning to update the manual as best I can and would be happy further research and include a bit on timecode and DTS/PTS etc.

Q: Do artists really need to know which timecode to use to have reliable motion tracking?

Short answer: No. or maybe if they are having sound sync issues.

My understanding is that it's mainly for when editors are cutting between cameras with different timecodes and sync.. and which shouldn't happen in a modern setting anyways.

Q: Is there a common timecode which will "just work"? Is it possible to guess it from video itself somehow?

For motion match and most editing we don't really care about the timecode. All we need to worry about is framerate. Timecode should only be a concern editing two synchronized recordings ( and even then ffmpeg should include a start recording time which should work for this).
For display in the VSE I think it's just a nicer way to view the timing in integer numbers: HH:MM:SS:Frame or HH:MM:SS;FF for drop (note the semi-colon for drop ).
To answer your question: we can guess if it's drop frame if it's 29.97 or 59.94i. but we can't really know if it's run-free or record run ( real wall-clock time or accumulated record time).

C: "Videos appear on frame 0..." - This will be good to address, but as a separate patchset.

Sound good.

C; "improved variable names, plus changed _all_ camelcase names to underscored versions." That's cool. Usually we prefer such rename happen as separate change...

I understand. I was changing such a large portion I thought it might make a review easier in this case. I don't really have that excuse for indexer.c . I've got everything in separate commits and can rebase to break this up if necessary.

Sybren,

Thanks too for your comments.

Indexing: Yes, I agree, leave for a separate patch.

Code changes:
I promise I'm not trying to ResurrectTheAlmightyCh'thulu ! ;)
Will address code issues and upload hopefully in the next few days.

Quick note: Downloaded FFMPEG test videos ( accessible via ffmpeg source: make fate-rsync ).
I'll hold off on updating code here a few more days to test these edge cases.

source/blender/imbuf/intern/anim_movie.c
506	I misled you with wrong naming convention for struct. Renamed to FramePtsRecord and kept struct here.

Hi @Justin Jones (jjones780), how are things going with this patch?

Hi Sybren, Thanks for asking. I was able to get back to this after an unforeseen break.
I've tested quite a few edge cases thanks to the ffmpeg "fate" test videos - quite a learning experience.

I've added code to warn the user with a call to WM_report if there is a frame decode mismatch detected.
This was done in the gui code higher up in the call chain and I will be interested in your feedback... I suspect there is a better way.
I hope to get things cleaned up and code here soon.

Changes and tuning thanks to ffmpeg fate-suite videos.
- handles a greater range of edge cases.
- warns user if a frame seek mismatch occurs.
- more efficienct seeking with fallback to earlier seek.
- change deprecated av_free_packet to unrefs.
- use stream specific pts timing for seeks.

Please have a look at:
I'm not sure the approach I used to warn the user is appropriate ( checking the anim structure from the gui code in the space_clip/* files ).
CMakeLists.txt change required to access anim struct. OK?

Note: Indexer.c
Indexer.c was only changed to update some deprecated ffmpeg calls. Other changes are only superficial variable renaming. I do plan to look into updating the indexer code as time permits ( to get rid of nonsensical options ). I forgot to re-test with indexing on this commit.

Note2: ToDo
There are a few more deprecated ffmpeg functions best left for another commit ( after I update libs ).

Harbormaster completed remote builds in B7173: Diff 22832.Mar 16 2020, 11:35 AM

Peter Fog (tintwotin) added projects: BF Blender, Video Sequencer, VFX & Video.Mar 16 2020, 12:06 PM

For anyone interested:

The H264 format breaks the assumption that you can seek to an IFrame and decode a complete video frames starting from there.
H264 has PPS and SPS settings which can change during the video and are required to decode an IFrame.
AnnexB was added to the h264 spec to address this.
FFMPEG's h264_mp4toannexb filter forces the SPS and PPS to be repeated at seek points.

I'm not sure the approach I used to warn the user is appropriate

I need to catch up here a bit to give some meaningful advice.

Is this warning something what happens once you open the video? Or do you intend to do this during playback? The latter one would be super annoying for users, especially if the message doesn't give any clues about how to make frame seek reliable.

Isn't this also something what is not supposed to happen if the timecode is properly built?

"Clean up clip user notification warnings/errors"

I had a fresh look at my clip_* changes and realized the method I was using might be the right way to go about it.

What had me second guessing myself was including across intern directories and changing CMake files. Am I breaking any Blender standard practices there?

Harbormaster completed remote builds in B7185: Diff 22852.Mar 17 2020, 7:04 AM

In D6392#170943, @Sergey Sharybin (sergey) wrote:

Is this warning something what happens once you open the video? Or do you intend to do this during playback? The latter one would be super annoying for users, especially if the message doesn't give any clues about how to make frame seek reliable.

The warning can occur when the video is first loaded but may also occur during playback. To avoid annoying the user it only produces the warning once and it appears as a passive orange highlighted message at the bottom of the blender window. I've added suggestions to the warning message. The user can also see it in the info window if they know to look for it there (hmm):

Isn't this also something what is not supposed to happen if the timecode is properly built?

True, it should but this patch should make timecode indexing redundant and no longer cause new users problems... or warn them if their video does need an index or other solution (see below).

I don't see this new mismatch frame seek warning come up anymore except for the h264 non-annexb test videos ( hence my hope that indexing is now generally redundant - fingers crossed ). If there's anything I've learned about video standards is that they are not well adhered to... and this warning should make the user aware if problems do occur.

Note: Building a timecode index will not help with non-annexb h264 encoded videos ( The user needs to be told to transcode ) because the iFrames are not all-inclusive ( required SPS/PPS data is separate) and thus is a decoding issue not a seek issue.
TODO: call up the H264 SPS/PPS specific warning I just added instead of it being caught by the mismatch warning.

Added more UI notifications of ffmpeg frame seek issues.

Harbormaster completed remote builds in B7358: Diff 23241.Mar 29 2020, 11:13 AM

I don't have any further additions to this patch except any requested by review.

What this patch accomplishes:

Frame seek accuracy under more conditions (using a look ahead buffer).
Warn the user when frame accuracy can't be achieved.
More robust decoding - more edge cases handled (thanks to FFMPEG test files - but also no thanks... it's a black hole).
Moved to FFMPEG api calls which decouple packet input to frame output. ( yes, should've been a separate patch ).

Commit message: Use refcounted_frames so ffmpeg can properly manage internal buffers. Use AVDictionary to set option.

I've had a nagging feeling for a while now and recently ( after a break and fresh look ) found my answer. The frame objects are not guaranteed to have a static copy of the decoded frame after another call to avcodec_receive_frame(). More likely if using multithreaded and/or hardware decoding. FFMPEG provides a refcounted_frames option in which case it will make copies if it can't guarantee the codec won't invalidate it.
Glad I got around to researching it. Yikes. No other outstanding issues sticking in my head although it would be nice to get this into part of an experimental build so users could test it.

Note: av_dict_set(&opts, "workaround_bugs", "1", 0) indicated the option was no longer valid.

Harbormaster completed remote builds in B7986: Diff 24607.May 11 2020, 12:00 PM

Evan Wilson (EAW) awarded a token.May 14 2020, 8:47 PM

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).

Justin Jones (jjones780) edited the summary of this revision. (Show Details)Jun 29 2020, 8:48 AM

Updating D6392: FFMPEG frame seek bug fix
Rebase

Harbormaster completed remote builds in B9671: Diff 27950.Aug 20 2020, 1:04 AM

Moved to FFMPEG api calls which decouple packet input to frame output. ( yes, should've been a separate patch ).

This has gotten quite a big patch, do you think there is any chance of breaking it down into a few smaller bits that can be committed individually?

I'll look into getting the ffmpeg call upgrades separated out and whatever else is straightforward and see how it looks from there.
I'll find the time to do more serous carving eventually.

Peter Fog (tintwotin) mentioned this in D8393: Media transform redesign.Sep 15 2020, 10:06 AM

rebase to master
Note: In process of breaking this up into smaller patches (as time allows).

Harbormaster completed remote builds in B12967: Diff 34119.Feb 18 2021, 10:54 AM

We're discussing ffmpeg seeking in the VSE chat currently. Maybe you would like to join?
https://blender.chat/channel/vse

I've got this broken into 3 commits. How do I best present them for discussion/review... in a separate branch? One at a time? Ho should I go about uploading them? I don't believe I have required permissions.

Commits ready to discuss/review:

contains the code to notice misaligned pts values for blender frames and notify the user.
dedicated to variable name changes to existing variables ( as per requests above )
hard to break down further, includes upgrade to new ffmpeg method calls and look-ahead frame decode solution.

fix merge issues

Harbormaster completed remote builds in B12994: Diff 34162.Feb 19 2021, 10:53 AM

3/3 of old original D6392. See D10471 and D10472 for parts 1/3 and 2/3 respectively.

Harbormaster completed remote builds in B12997: Diff 34166.Feb 19 2021, 11:45 AM

Justin Jones (jjones780) added a parent revision: D10472: commit 2/3 of breakdown of D6392 Variable name changes only - no functional code changes.Feb 19 2021, 11:54 AM

Justin Jones (jjones780) mentioned this in D10472: commit 2/3 of breakdown of D6392 Variable name changes only - no functional code changes.Feb 19 2021, 12:09 PM

Justin Jones (jjones780) mentioned this in D10471: commit 1/3 of breakdown of D6392.

Note that indexer.c changes are ONLY related to upgrading to the newer ffmpeg calls and could be considered a separate revision.
Changes to anim_movie.c also include this upgrade as part of a larger fix.

@Justin Jones (jjones780) Hi, I have been working on some changes in seeking see D10644. These were intended just as refactor/cleanup with only positive impact on functionality if any. The remove preseek patch is actually combined with another refactoring patch D10638. So technically it's just handful of lines. Juicy bit is ffmpeg_can_scan() which decides what method of seeking will be used.

Just for curiosity sake I have tried to reproduce issue in T72347 and to me it looks like my approach will fix this issue as well. I could reproduce off by 1 issues but not sure if these are same as you described. Can you check it out and let me know if it indeed fixes the issue and what do you think about such approach? Just note that my patch has not been polished yet so sometimes frame is not decoded(usually it is first frame). I can see that there are other cases that my patch wont cover, but question rather is if it is "compatible" with what you have done here.

Also just for info, there is patch for porting code to new FFmpeg API in D10338, it's probably best to commit that separately. And in e4f347783320 there are changes that could allow making timecode indexes much faster, at least for some (most?) files.

So far I have checked this patch only briefly, I will look at code more closely to understand your changes.

@Richard Antalik (ISS)

In D6392#272869, @Richard Antalik (ISS) wrote:

@Justin Jones (jjones780) Hi, I have been working on some changes in seeking see D10644.

Hi Richard, That all sounds great! I'd be happy to have a look and see if I can contribute - very busy at the moment but hope to have some helpful feedback soon.

Richard Antalik (ISS) mentioned this in D13583: FFmpeg: Add VFR media support.Dec 15 2021, 6:17 AM

Richard Antalik (ISS) mentioned this in rBf0a3d2beb23e: FFmpeg: Add VFR media support.Jun 27 2022, 4:58 PM

FFMPEG frame seek bug fix
Needs ReviewPublic
Actions

Details

FFMPEG frame seek bug fix

Replicating:

Background:

Strategy:

Compatibility with current Proxy timecode files:

Code changes ( all within source/blender/imbuf/intern/anim_movie.c and IMB_movie.h ):

Notes:

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 20685

source/blender/imbuf/intern/IMB_anim.h

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

FFMPEG frame seek bug fixNeeds ReviewPublicActions

Details

FFMPEG frame seek bug fix

Replicating:

Background:

Strategy:

Compatibility with current Proxy timecode files:

Code changes ( all within source/blender/imbuf/intern/anim_movie.c and IMB_movie.h ):

Notes:

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 20685

source/blender/imbuf/intern/IMB_anim.h

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

FFMPEG frame seek bug fix
Needs ReviewPublic
Actions

Revision Contents
Changeset List