Page MenuHome

FFMPEG frame seek bug fix
Needs ReviewPublic

Authored by Justin Jones (jjones780) on Dec 11 2019, 10:45 AM.

Details

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.

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:

When decoding compressed video frames we have very few "knowns" unless we want to get into working with specific compression codecs individually.
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 not have an image assocated with them, therefore the only way to know if we have the associated image is to decode until we have a PTS for the next timeframe.
Without doing a pre-indexing we cannot think in terms of frames from the decoder.. we have to use timestamps only. We should be working in the time domain ( as opposed to frame domain ) in order to minimize audio sync issues.

The time frame we are searching may NOT have its own image ( due to compression or slow encoding ) so we need to keep showing the previous one ( which may be more than one video frame ago ).
The time frame we are searching may encompass multiple images. We will only show one... and let an indexing method deal with this in a smarter way if the user deems necessary ( it might be appropriate to show it in a missed frame before).

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.

Don't worry about finding i-frames - FFMPEG has internal lookups to do this efficiently ( or so it seems ).

If we search forward and stop when we have a pts PAST our current timeframe then the frame before is the proper frame. This handles timing gaps etc.

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 ). Consider that this may be undesireable as proxies may put frame numbers ahead of timing. Needs testing.

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?? ).
Future:

A) Use newer FFMPEG functions where we are using deprecated ones.

B) Speed up motion matching in reverse by postprocessing and buffering the forward scan (from the iframe to current frame).

C) Out of sync motion trackers
We need to re-align motion tracking data ( or warn user and delete some/all ?) if the timecode index results in different frame mapping.

Different Timestamp Proxy's are all the same length in frames (but this should not necessarily be true in order to keep timing for sync with audio). Gap-less indexes will have less frames if the recording device was unable to encode at a consistent frame rate or if multiple frames were compressed into one. The new code accomodates this to a point: It re-initializes arrayFrame2PTS BUT the motion tracking already performed will not be touched and so may be out of sync.
No warning is given to the user. Documentation should state that indexing should not be changed after tracking until changes are made to accomodate this ( i.e. the tracking is linked to the frame's pts ??).

D) Consider having blender work with variable frame rate i.e. using a (pts) timestamp within a blender frame for e.g. physics related calculations. Worth the trouble?? Only for video editing use? How would this affect time-modifying scripts etc.

Other benefits from this bug fix:

Fewer or less severe auidio-sync issues?

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 independant of timing can still be done with a proxy index although stretching that back out so the timing is right would still require more changes. Blender would need to handle variable frame rates to make this really worthwhile.

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?

Diff Detail

Repository
rB Blender
Branch
ffmpeg_pts (branched from master)
Build Status
Buildable 6172
Build 6172: arc lint + arc unit

Event Timeline

Justin Jones (jjones780) retitled this revision from re: Issue T72347 to FFMPEG frame seek bug fix.Dec 11 2019, 11:07 AM
Justin Jones (jjones780) edited the summary of this revision. (Show Details)
Justin Jones (jjones780) edited the summary of this revision. (Show Details)

I see that arc squashed the commits :(
I'm attaching the first commit (39e37c9) - I had referenced it above.

Might as well put them all here... if someone ( future me ? ) prefers to have things broken down a bit.
39e37c9db56 Consistency check for frame scanned pts - keep track of searched and found. Log av_log error if searched or found mismatch from one scan to next! Use last found for new scan if searched is the same but found is different.


91548d35d2d Move next_pts check into frame scan

8fd83519780 Output final frames at eof. Calc number of frames from time values.

d7d51b976d9 Enable building of timecode without doing proxy images.

980f6c0340c Reinit arrayFrame2PTS when type of proxyindex changes

7150263fa85 Cleanup

D) Consider having blender work with variable frame rate i.e. using a (pts) timestamp within a blender frame for e.g. physics related calculations. Worth the trouble?? Only for video editing use? How would this affect time-modifying scripts etc.

Just a few thoughts to this point:
There are certainly users, that would appreciate VFR support, but I didn't consider this to be priority. Sequencer already have time-altering effect, so I want to create method of mapping frame you see in preview to frame number in domain of the source media (because we currently use frame numbers).

As for implementation - I would check if there is any convention of positioning elements in time with sub-frame accuracy and work from there. I only know, that curves can use frame and decimal fraction. We could also use time directly. It may be good idea to generalize and document approach in cases like this, but I am not sure at this point.
Then on FFMPEG side there should be layer, that translates Blender time (or what the convention may dictate) to pts and vice-versa. Perhaps we could use that to retrieve stream length, because now it seeems, that length is calculated only from amount of frames.

In any case, to me this sounds like effort for it's own patch

To patch itself:
I need to read some FFMPEG docs first. I may have time to do that, depends how quickly will this be aproved. So don't wait for my approval necessarily.

Just a few thoughts to this point:
There are certainly users, that would appreciate VFR support, but I didn't consider this to be priority. Sequencer already have time-altering effect, so I want to create method of mapping frame you see in preview to frame number in domain of the source media (because we currently use frame numbers).
As for implementation - I would check if there is any convention of positioning elements in time with sub-frame accuracy and work from there. I only know, that curves can use frame and decimal fraction. We could also use time directly. It may be good idea to generalize and document approach in cases like this, but I am not sure at this point.
Then on FFMPEG side there should be layer, that translates Blender time (or what the convention may dictate) to pts and vice-versa. Perhaps we could use that to retrieve stream length, because now it seeems, that length is calculated only from amount of frames.
In any case, to me this sounds like effort for it's own patch

I agree. It's outside of the scope of this patch.
The PTS info is available going forward.
I don't see that python scripts could use it though ( if we made it available)... no easy way to specify pts when encoding video?

To patch itself:
I need to read some FFMPEG docs first. I may have time to do that, depends how quickly will this be approved. So don't wait for my approval necessarily.

I have to mention this link: Digital Video Introduction

More relevant ones you already have:
The blender-git/lib/linux_x86_64/ffmpeg/include/libavcodec/avcodec.h file has a lot of documentation in it.
Here's the relevant FFMPEG doxygen page

I'm looking into replacing avcodec_decode_video2 with avcodec_receive_frame() because avcodec_decode_video2 is depreciated.

I'm looking into replacing avcodec_decode_video2 with avcodec_receive_frame() because avcodec_decode_video2 is depreciated.

I've already got that done in the files I'm looking at (anim_movie.c and indexer.c )
I was going to wait a bit more and upload these changes but will do so now so that you can have a look. I'd appreciate any comments you might have.

I've only hacked it in order to do some testing... I'm not sure I can see the forest through the trees at this point to better organize it. Or maybe it's workable?
Any comments on the refcounting? What about the anim_movie.c:1318 "Could it be a bug in FFmpeg?" from earlier code... I need to look into if this is still a concern with newer ffmpeg.

I have NOT done the upgrade from avcodec_encode_video2 in blender/source/blender/blenkernel/intern/writeffmpeg.c yet.

indexer: I've compared the resulting proxy data files with files produced before these changes and they are binary equivalent (for at least one video). More testing to do.

  • reinstate commented line from testing.
  • replace deprecated ffmpeg calls
  • handle EAGAIN result from avcodec_send_packet
  • upgrade deprecated call to write proxy videos
  • cleanup

Just some general points of an initial review:

  • We follow snake_style (not CamelCase) for variables and function names.
  • The ffmpeg tests are not passing (run regression test ctest -R ffmpeg). Since the failing files are actually synthetically generated the number of frames should match exactly.
  • See inlined comments.

Generally the direction seems fine, just need to do a deeper review and testing.

source/blender/imbuf/intern/anim_movie.c
494

use static qualifier.

502

Same as above.

954

anim->re_use_packet = true;

958

anim->re_use_packet = false;

1242

anim->re_use_packet = false;

source/blender/imbuf/intern/indexer.c
948

context->re_use_packet = true;

952

context->re_use_packet = false;

Justin Jones (jjones780) updated this revision to Diff 20365.EditedDec 18 2019, 7:27 AM

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.

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.

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.

Justin Jones (jjones780) updated this revision to Diff 20405.EditedDec 19 2019, 3:06 AM
  • 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.

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.

Justin Jones (jjones780) updated this revision to Diff 20643.EditedJan 6 2020, 1:25 PM

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

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/

Justin Jones (jjones780) updated this revision to Diff 20685.EditedJan 8 2020, 12:30 PM

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?

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 :)

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_packetreuse_packet, and on the suggested split between cleanup and functional changes.

source/blender/imbuf/intern/indexer.c
606–611

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–960

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.

Justin Jones (jjones780) marked 15 inline comments as done.Jan 24 2020, 11:11 PM

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?

Justin Jones (jjones780) marked an inline comment as done.Mar 2 2020, 12:38 PM

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.

Justin Jones (jjones780) updated this revision to Diff 22832.EditedMon, Mar 16, 11:35 AM
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 ).

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?

Justin Jones (jjones780) updated this revision to Diff 22852.EditedTue, Mar 17, 7:04 AM

"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?

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.

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

What I hope 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 ).