WIP: FFmpeg: Remove timecode support #108059

Richard Antalik · 2023-05-19T00:14:07+02:00

Richard Antalik commented

2023-05-19 00:14:07 +02:00

Remove index building code from proxy system, rename functions and types
referring to indexing to only refer to proxy building. Purpose of this
change is to decrease code complexity and needs for maintenance.

Currently, timecode system is in working order. However it is no longer
serving it's purpose - to correct seeking in video files.

Since double buffering was implemented for ffmpeg (f0a3d2beb2), there
were minimum amount of reports concerning seeking functionality. These
were all resolved and for last 7 months I haven't seen any such report.

TODO:
copy PTS/DTS from source to proxy files, otherwise lengths may not match.

Remove index building code from proxy system, rename functions and types referring to indexing to only refer to proxy building. Purpose of this change is to decrease code complexity and needs for maintenance. Currently, timecode system is in working order. However it is no longer serving it's purpose - to correct seeking in video files. Since double buffering was implemented for ffmpeg (f0a3d2beb23e), there were minimum amount of reports concerning seeking functionality. These were all resolved and for last 7 months I haven't seen any such report. TODO: copy PTS/DTS from source to proxy files, otherwise lengths may not match.

Richard Antalik added 1 commit 2023-05-19 00:14:17 +02:00

3c7e2a259b FFmpeg: Remove timecode support

Remove index building code from proxy system, rename functions and types
referring to indexing to only refer to proxy building. Purpose of this
change is to decrease code complexity and needs for maintenance.

Currently, timecode system is in working order. However it is no longer
serving it's purpose - to correct seeking in video files.

Since double buffering was implemented for ffmpeg (f0a3d2beb2), there
were minimum amount of reports concerning seeking functionality. These
were all resolved and for last 7 months I haven't seen any such report.

Richard Antalik requested review from Sergey Sharybin 2023-05-19 00:15:30 +02:00

Richard Antalik requested review from Sebastian Parborg 2023-05-19 00:15:30 +02:00

Sergey Sharybin commented

2023-05-19 12:00:54 +02:00

Currently, timecode system is in working order. However it is no longer serving it's purpose - to correct seeking in video files.

I am confused by the wording. If it is in working state, why does it not serve the purpose?

Is the double buffering guarantee to be at least as good as time code? While it might not be as noticeable for VSE, 1 frame offset while doing motion tracking is fatal.

Also, from looking around the bug tracker and trying various files it seems that time code also helps with videos like from #44397.

I also have another video which shows seek video. Unfortunately, I've lost its credit, so will sent directly to you.

> Currently, timecode system is in working order. However it is no longer serving it's purpose - to correct seeking in video files. I am confused by the wording. If it is in working state, why does it not serve the purpose? Is the double buffering guarantee to be at least as good as time code? While it might not be as noticeable for VSE, 1 frame offset while doing motion tracking is fatal. Also, from looking around the bug tracker and trying various files it seems that time code also helps with videos like from #44397. I also have another video which shows seek video. Unfortunately, I've lost its credit, so will sent directly to you.

Richard Antalik commented

2023-05-20 02:58:59 +02:00

Currently, timecode system is in working order. However it is no longer serving it's purpose - to correct seeking in video files.

I am confused by the wording. If it is in working state, why does it not serve the purpose?

Because my aim was to make seeking work out of the box and my impression was, that we have reached this goal.

Is the double buffering guarantee to be at least as good as time code? While it might not be as noticeable for VSE, 1 frame offset while doing motion tracking is fatal.

I am aware, that for tracking these issues are critical. To me there is 1 known case where seeking with double buffering can fail. This is when there is "delay" between PTS and DTS, which we do correct by adding (arbitrary) offset to seek timestamp. This is because how ffmpeg does seeking internally and can't be resolved in any other way to my knowledge. So far there are no open issues for this case.

So in short there are still cases where timecodes would be only option to seek in some files precisely. I would bet, that there are files, where even timecodes won't help :)

To add bit more nuance here, when you build proxies with this change, we can mitigate pts/dts delay issue and therefore make perfectly seekable file. Therefore at least when proxy is used, I could guarantee these will work as good as with timecodes.

Also, from looking around the bug tracker and trying various files it seems that time code also helps with videos like from #44397.

I also have another video which shows seek video. Unfortunately, I've lost its credit, so will sent directly to you.

For #44397 I don't see anything wrong here - the file was recorded at 2.5 fps, just seems, that recording software specified 30fps. If you check actual time in the video, it matches time in timeline. Same goes for VLC anf ffplay.

The file you've sent to me I see it's not seeking well. For MTS files, we've had to implement own low-ish level seeking function ffmpeg_generic_seek_workaround(), because ffmpeg itself soes not support seeking to keyframe with this format. Will check bit more in detail what happens there, It should not fail the way it does now at least.
That said, seeking in this file fails even with timecodes. It only works when transcoded proxy file is used. This change would copy timestamps from source to proxy, however it would change container, so seeking issues would still be resolved.

Personally I don't have very strong opinion about having support for timecodes, but I would remove the system. Or I would be open to level of deprecation where the system is only available when enabled as experimental feature for some time period.
On one hand there have been bugs in timecode builder itself that are somewhat confusing to debug and fix, and I would say, that if reasonable algorithm can not seek in some files, I would suggest transcoding these into more reasonable formats.
On the other hand we can offer timecodes as solution, but it is not immediately obvious for the user to use it or how to use it. So we still end up with bug report, that has to be investigated. I know, that it is different issue in itself. These are just my thoughts. Even concept of timecodes itself can be confusing for user, compared to just "bad format".

I will add WIP tag here since I forgot, that we would have to copy PTS/DTS from source to proxy files, otherwise lengths may not match.

> > Currently, timecode system is in working order. However it is no longer serving it's purpose - to correct seeking in video files. > > I am confused by the wording. If it is in working state, why does it not serve the purpose? Because my aim was to make seeking work out of the box and my impression was, that we have reached this goal. > Is the double buffering guarantee to be at least as good as time code? While it might not be as noticeable for VSE, 1 frame offset while doing motion tracking is fatal. I am aware, that for tracking these issues are critical. To me there is 1 known case where seeking with double buffering can fail. This is when there is "delay" between PTS and DTS, which we do correct by adding (arbitrary) offset to seek timestamp. This is because how ffmpeg does seeking internally and can't be resolved in any other way to my knowledge. So far there are no open issues for this case. So in short there are still cases where timecodes would be only option to seek in some files precisely. I would bet, that there are files, where even timecodes won't help :) To add bit more nuance here, when you build proxies with this change, we can mitigate pts/dts delay issue and therefore make perfectly seekable file. Therefore at least when proxy is used, I could guarantee these will work as good as with timecodes. > Also, from looking around the bug tracker and trying various files it seems that time code also helps with videos like from #44397. > > I also have another video which shows seek video. Unfortunately, I've lost its credit, so will sent directly to you. For #44397 I don't see anything wrong here - the file was recorded at 2.5 fps, just seems, that recording software specified 30fps. If you check actual time in the video, it matches time in timeline. Same goes for VLC anf ffplay. The file you've sent to me I see it's not seeking well. For MTS files, we've had to implement own low-ish level seeking function `ffmpeg_generic_seek_workaround()`, because ffmpeg itself soes not support seeking to keyframe with this format. Will check bit more in detail what happens there, It should not fail the way it does now at least. That said, seeking in this file fails even with timecodes. It only works when transcoded proxy file is used. This change would copy timestamps from source to proxy, however it would change container, so seeking issues would still be resolved. Personally I don't have very strong opinion about having support for timecodes, but I would remove the system. Or I would be open to level of deprecation where the system is only available when enabled as experimental feature for some time period. On one hand there have been bugs in timecode builder itself that are somewhat confusing to debug and fix, and I would say, that if reasonable algorithm can not seek in some files, I would suggest transcoding these into more reasonable formats. On the other hand we can offer timecodes as solution, but it is not immediately obvious for the user to use it or how to use it. So we still end up with bug report, that has to be investigated. I know, that it is different issue in itself. These are just my thoughts. Even concept of timecodes itself can be confusing for user, compared to just "bad format". I will add WIP tag here since I forgot, that we would have to copy PTS/DTS from source to proxy files, otherwise lengths may not match.

Richard Antalik changed title from ~~FFmpeg: Remove timecode support~~ to WIP: FFmpeg: Remove timecode support

2023-05-20 02:59:41 +02:00

Sebastian Parborg commented

2023-05-22 11:07:00 +02:00

I just want to chime in and say that I fully agree with Richard that the timecodes are superfluous and makes the code harder to maintain and needlessly complicated.

When Richard and I did work to improve and clean up the seeking code, it was kinda obvious to me that the timecodes were implemented to workaround the (at the time) very crappy and error prone seeking code.

However after Richard and I cleaned up and improved the seeking code, we came to the conclusion that it was not needed anymore now that seeking in is a good state.

As Richard pointed out, if our seeking code now fails, timecodes will most likely not help and the only way to solve it is to re-encode the video.
We can tell when seeking fails, so I think it would be nice to report this to the user somehow so they know that the video they have is crappy.

I would even go as far to say that videos that fails to seek are probably not up to spec with the video format. At least for the most common formats used today in 2023. While I think we should support as many video files as possible, I don't think we should try to bend over backwards to read badly encoded files. (The same way we don't support reading mangled .fbx or .obj files)

I just want to chime in and say that I fully agree with Richard that the timecodes are superfluous and makes the code harder to maintain and needlessly complicated. When Richard and I did work to improve and clean up the seeking code, it was kinda obvious to me that the timecodes were implemented to workaround the (at the time) very crappy and error prone seeking code. However after Richard and I cleaned up and improved the seeking code, we came to the conclusion that it was not needed anymore now that seeking in is a good state. As Richard pointed out, if our seeking code now fails, timecodes will most likely not help and the only way to solve it is to re-encode the video. We can tell when seeking fails, so I think it would be nice to report this to the user somehow so they know that the video they have is crappy. I would even go as far to say that videos that fails to seek are probably not up to spec with the video format. At least for the most common formats used today in 2023. While I think we should support as many video files as possible, I don't think we should try to bend over backwards to read badly encoded files. (The same way we don't support reading mangled `.fbx` or `.obj` files)

Sergey Sharybin commented

2023-05-22 15:29:11 +02:00

From just a quick look around at files which I knew were problematic does not convince me the seeking code is reliable enough. To me the UX is more important than clean-ness of code. After all, we are making 3D creation tool for artists, not for developers.

I much rather see simplification of the process when people can do VFX on the media from their devices in a reliable way. Which means, if some formats are known to have problems the proper solution is to warn the user about it, and ask whether they'd desire Blender to transcode the media to a compliant format.

From just a quick look around at files which I knew were problematic does not convince me the seeking code is reliable enough. To me the UX is more important than clean-ness of code. After all, we are making 3D creation tool for artists, not for developers. I much rather see simplification of the process when people can do VFX on the media from their devices in a reliable way. Which means, if some formats are known to have problems the proper solution is to warn the user about it, and ask whether they'd desire Blender to transcode the media to a compliant format.

This pull request has changes conflicting with the target branch.

source/blender/blenkernel/intern/image.cc
source/blender/blenkernel/intern/movieclip.c
source/blender/blenloader/intern/versioning_260.c
source/blender/editors/space_clip/clip_ops.cc
source/blender/editors/space_image/image_buttons.c
source/blender/editors/space_image/image_ops.c
source/blender/editors/space_sequencer/sequencer_drag_drop.c
source/blender/editors/space_sequencer/sequencer_proxy.c
source/blender/imbuf/CMakeLists.txt
source/blender/imbuf/IMB_imbuf.h

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u bye-indexer:iss-bye-indexer

git checkout iss-bye-indexer

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: FFmpeg: Remove timecode support #108059

Checkout