Details

Reviewers

Sergey Sharybin (sergey)
Richard Antalik (ISS)
Jeroen Bakker (jbakker)

Group Reviewers

VFX & Video

Commits

rBf522e8db2288: Cleanup: Remove deprecated variables and functions calls from our ffmpeg code
rB8d6264ea12bf: Cleanup: Remove deprecated variables and functions calls from our ffmpeg code

Summary

Part of T73586

I haven't tested this thoroughly so things might be broken.
However, our automated tests pass, so that's something :)

Perhaps we should also cleanup more of the LIBAVCODEC_VERSION_MAJOR checks and bump our supported minimum ffmpeg version?
For the check I removed in this patch I noticed that the function avcodec_encode_audio is competently removed in the recent 4.2.x and 4.3.x versions.
In addition to this, seems like most checks are for version from 2012-2014, so I think it should be safe to clean these up.
I want to ask first before I do it though as that was not part of the code quality task.

Note that this is not the final version of this patch.
There are a few temporary changes and TODOs.
But I wanted to check if what I have done so far is ok and doesn't break any specific code path.

Diff Detail

Event Timeline

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

We do have an automated test for duration detection. We can render a tiny movie and add it to the automated sequence_editing suit.
While adding an automated test would be lovely, doing a preliminary test with any video file prior to the patch submission is something reviewers are expecting to happen ;)

But I wanted to check if what I have done so far is ok and doesn't break any specific code path.

Well.... The code looks a typical API update change, and checking that will probably take exact same amount of time as it took you to investigate how to change the API. What is not OK is that video strips no longer working :(

This revision now requires changes to proceed.Feb 8 2021, 9:49 AM

Re-checked my file that got wrong length, it has wrong length even in older Blender versions, in VLC it plays fine, so I assumed this must have been regression in this patch, because I have used it for testing previously. Pehaps I have damaged it somehow but don't see any warning or error messages in VLC either.

Sorry for not checking more in detail, haven't got much time yesterday, I had this patch compiled already and just wanted to quickly do something in VSE when I found these issues.

Aargh, I mistyped patch when checking out and switched to cmd too fast. so I have edited previous coment.

I still get bad image

Will check the issue

Here is fix for broken decoding:

diff --git a/source/blender/imbuf/intern/anim_movie.c b/source/blender/imbuf/intern/anim_movie.c
index 3ceed1fd0d5..e10a08a05f4 100644
--- a/source/blender/imbuf/intern/anim_movie.c
+++ b/source/blender/imbuf/intern/anim_movie.c
@@ -912,2 +912,2 @@ static int ffmpeg_decode_video_frame(struct anim *anim)
-      avcodec_send_frame(anim->pCodecCtx, anim->pFrame);
-      anim->pFrameComplete = avcodec_receive_packet(anim->pCodecCtx, &anim->next_packet) == 0;
+      avcodec_send_packet(anim->pCodecCtx, &anim->next_packet);
+      anim->pFrameComplete = avcodec_receive_frame(anim->pCodecCtx, anim->pFrame) == 0;
@@ -944,2 +944,2 @@ static int ffmpeg_decode_video_frame(struct anim *anim)
-    avcodec_send_frame(anim->pCodecCtx, anim->pFrame);
-    anim->pFrameComplete = avcodec_receive_packet(anim->pCodecCtx, &anim->next_packet) == 0;
+    avcodec_send_packet(anim->pCodecCtx, &anim->next_packet);
+    anim->pFrameComplete = avcodec_receive_frame(anim->pCodecCtx, anim->pFrame) == 0;

I have seen some issue with encoding as well so will have a look.

Why do you free codec opened in alloc_video_stream()? This causes write_video_frame() to fail to encode frames.

You probably could reopen codec each time, but you have to provide AVDictionary which could be kept in FFMpegContext, but I think that you can just flush codec instead and keep it open.

source/blender/blenkernel/intern/writeffmpeg.c
734	`context->video_stream` is always `NULL` here
1180–1181	I don't think this is needed since `avcodec_free_context` should free everything. In any case it causes `avcodec_free_context` to fail.

Richard Antalik (ISS) mentioned this in D10394: Improve proxy building performance.Feb 24 2021, 12:30 AM

Updated and it seems to be working now.

In D10338#260312, @Richard Antalik (ISS) wrote:

Why do you free codec opened in alloc_video_stream()? This causes write_video_frame() to fail to encode frames.

You probably could reopen codec each time, but you have to provide AVDictionary which could be kept in FFMpegContext, but I think that you can just flush codec instead and keep it open.

I've now moved the codec contexts into FFMpegContext.
I also feel that it would be wasteful to create and destroy these for each frame.
So now we instead have them there and free them after the task is done.

Sebastian Parborg (zeddb) updated this revision to Diff 34813.Mar 5 2021, 4:19 PM

Seems like I spoke too soon. It seems like these changes makes frames near the end of the video get stuck in the output file.
(So the last few frames are skipped and replaced with the frame before them)

Fixed the flushing issue so now the end frames seems to be encoded and saved properly to the output file.
However the flushing function errors out. I'm not sure if that is harmless or if there is actually something wrong.

Sebastian Parborg (zeddb) updated this revision to Diff 34823.Mar 5 2021, 6:33 PM

Sebastian Parborg (zeddb) updated this revision to Diff 34824.Mar 5 2021, 6:37 PM

In D10338#268709, @Sebastian Parborg (zeddb) wrote:

In D10338#260312, @Richard Antalik (ISS) wrote:

You probably could reopen codec each time, but you have to provide AVDictionary which could be kept in FFMpegContext, but I think that you can just flush codec instead and keep it open.

I also feel that it would be wasteful to create and destroy these for each frame.

Well as I have learned later you couldn't really do that anyway...

So far here(P2015) are my changes to indexer.c as I have written inlines to crosscheck myself.

Will check rest of patch as well.

source/blender/imbuf/intern/indexer.c
499	This doesn't seem to work. Not sure why, I tried this in different patch and it caused problems. Here it causes `avformat_write_header()` to return error, which doesn't stop building process. But that's another issue. Reverting this change solved the issue.
525	I think this is supposed to be copied form source codec
551	this is definitely supposed to be copied form source codec
553	This condition also should compare parameters of source vs output codec and determine whether scaling or pixel conversion is needed. Now it is essentially `if (0)`
565–567	These must be source codec values.
616–618	If I understand docs(https://ffmpeg.org/doxygen/trunk/group__lavc__encdec.html) correctly, here we shouldn't care about return value of `avcodec_send_frame`.
619	`ret` is cast from bool here, so conditions below will never fail.
623	This should return 0 so when flushing for end of stream, it would exit the loop.
637	Looks like `av_interleaved_write_frame` is just happy to write even empty packets into file. `ret` variable is changed here as well and this causes that process will get stuck here.

Here are some comments for writeffmpeg.c file, these inlines apply to other equivalent places, refer to diff in P2016.

One technicality is that after avcodec_send_packet() avcodec_receive_frame() does not follow read output until AVERROR(EAGAIN) in this patch. In indexer.c this is possible to change easily. In anim_movie.c it would need bit more refactoring, not sure if I would do that in this patch.

Other than that, I think patch is fine.

source/blender/blenkernel/intern/writeffmpeg.c
331	if `ret` is non-zero this should probably set success to "false"
335	Returned here after some time and this got me again - `ret` is cast from bool
338	Not sure if I would use term "flush" here, as encoder will only allow you to read as many packets as necessary to free buffer for next packet. Also it's packets, not frames. I would say `/* No more packets available. */`
348	Rendering worked only because `av_interleaved_write_frame` returned error, because packet had invalid dts/pts. Otherwise it would end up in endless loop. I don't really like idea of `ret` being shared across more functions...
source/blender/imbuf/intern/anim_movie.c
917	Here read output until `AVERROR(EAGAIN)` scheme is probably not an option. At least I am not sure how it could be done.
948–949	this is read output until `AVERROR(EAGAIN)` scheme, but it's quite convoluted. At least it's commented though...
source/blender/imbuf/intern/indexer.c
951	This could follow read output until `AVERROR(EAGAIN)` scheme as well. It is quite unlikely that 2 frames could be read though.
971–980	This could follow read output until `AVERROR(EAGAIN)` scheme. Technically it already is, but it's not as clear.

Sebastian Parborg (zeddb) marked 4 inline comments as done.Mar 8 2021, 11:56 AM

Writer and Indexer feedback done.

Now the proxy system works without using the the deprecated codec variable.
I had to dig into the ffmpeg source code to notice that it failed because we didn't initialize codecpar.
So now when we do that, and fix the other things pointed out, it works again.

Will take a look at the anim_movie feedback now.

Sebastian Parborg (zeddb) added inline comments.Mar 8 2021, 5:29 PM

CMakeLists.txt
1535	I guess I can just remove this?
intern/ffmpeg/ffmpeg_compat.h
25	I think that we should remove all of the libav format checks here. (the versions they check here is ancient at this point, I don't think any supported Ubuntu LTS versions ship with these old versions either)
source/blender/imbuf/intern/indexer.c
616–618	Perhaps I'm blind, but where does it say that we don't have to check the return value? I mean, I don't see the harm in checking it in case there is an error so we can get better debug messages.

Sebastian Parborg (zeddb) updated this revision to Diff 34898.Mar 8 2021, 6:14 PM

To me, it seems like the the anim_movie logic is dependent on only decoding one frame at a time.

I'm unsure if we should or need to change this.

Sebastian Parborg (zeddb) marked 2 inline comments as not done.Mar 8 2021, 6:22 PM

Jeroen Bakker (jbakker) resigned from this revision.Mar 9 2021, 8:12 AM

In D10338#269446, @Sebastian Parborg (zeddb) wrote:

To me, it seems like the the anim_movie logic is dependent on only decoding one frame at a time.

I'm unsure if we should or need to change this.

Potentially, I will have to look at T72347 and attached patches.

From my point this patch looks fine. There was conflict with latest changes in indexer.c, P2038 is indexer.c after resolving these.

source/blender/imbuf/intern/indexer.c
616	This isn't audio frame.
616–618	Perhaps I'm blind, but where does it say that we don't have to check the return value? I mean, I don't see the harm in checking it in case there is an error so we can get better debug messages. I meant that passing empty packet to `avcodec_send_frame` won't return `AVERROR(EAGAIN)` or `AVERROR_EOF` Checking for other error states is indeed OK.

Richard Antalik (ISS) mentioned this in D6392: FFMPEG frame seek bug fix.Mar 18 2021, 3:02 AM

I updated the patch to the latest master commit and fixed an issue with ffmpeg 4.4.
(We need to set the audio channels and format in the audio frame)

Removed all outdated code from ffmpeg_compat.h

Sebastian Parborg (zeddb) updated this revision to Diff 36773.May 4 2021, 4:11 PM

Sergey Sharybin (sergey) added inline comments.May 4 2021, 4:16 PM

CMakeLists.txt
1549	Why those are removed? FFmpeg is not the only library which might trigger use of deprecated symbols. And in the future it might be so that some of the current FFmpeg API will be deprecated.

Sebastian Parborg (zeddb) added inline comments.May 4 2021, 4:27 PM

CMakeLists.txt
1549	Oh, I thought the reason this was here was that we had a lot of ffmpeg deprecation warnings that polluted the build log. (And because no one would fix this at the time the flag was added) I thought the idea behind this cleanup task was that we could remove this so that we can get warnings when deprecation happens. This way we are well prepared and know in advance when we need to update stuff. Otherwise we might get to a situation where we had no idea something was about to get removed and could do no planning for it. However I'm unaware of any previous discussions that has happened about this topic, so if you want me to add the flag back, I'll do it.

Sergey Sharybin (sergey) added inline comments.May 4 2021, 4:41 PM

CMakeLists.txt
1549	Think this flag predates FFmpeg API changes which were causing warnings nowadays. Also, keep in mind, those are only flags which take affect after `remove_strict_flags` (aka, the `extern/` folder). If some external library uses deprecated API of its dependency you don't want warning to be generated because it wouldn't really in our control to resolve that. The idea behind the task was to move to a non-deprecated API before it is too late (imagine if the API is removed before we started the port). As a bonus the compilation log will be more clean now :) Otherwise we might get to a situation where we had no idea something was about to get removed and could do no planning for it. Just to make it explicit: I do believe you will see deprecation warnings in our own code, outside of `extern/`.

Sebastian Parborg (zeddb) added inline comments.May 4 2021, 6:01 PM

CMakeLists.txt
1549	Ah, right. I didn't know that the ffmpeg files had been added to a `remove_strict_c_flags` macro. I will change back so the flag is still here in this file and remove the additions of the ffmpeg files to the `remove_strict_c_flags` macro.

Sergey Sharybin (sergey) added inline comments.May 4 2021, 6:08 PM

CMakeLists.txt
1549	I don't think FFmpeg files are covered with `remove_strict_c_flags`. If you don't have deprecation warning, I think something else is going on then, because I do have the deprecation warnings in our FFmpeg files.

Sebastian Parborg (zeddb) updated this revision to Diff 36778.May 4 2021, 6:18 PM

Sebastian Parborg (zeddb) marked 2 inline comments as done.May 4 2021, 6:22 PM

Sebastian Parborg (zeddb) added inline comments.

CMakeLists.txt
1549	Look at the new cmake changes to the cmake files in blenkernel and imbuf ;) I did not get any deprication warnings properly until I removed those.

Sebastian Parborg (zeddb) updated this revision to Diff 36780.May 4 2021, 6:32 PM

Sebastian Parborg (zeddb) marked 2 inline comments as done.

I think this might be good to go now.

I'm currently writing some rudimentary import/export automated tests as nearly all of the severe breakages I've run into wasn't caught by our tests at all.
But that will be an patch.

I also tested this on the ffmpeg git master branch, but they changed some internal variables to be const so more things needs to be rewritten for that to work.
However I feel that is for an other patch as this one has grown quite a bit.

Does this look good to you guys @Sergey Sharybin (sergey) @Richard Antalik (ISS) ?

wasn't caught by our tests at all

Our test is minimalistic. Based on some issues I've run into last time i've touched the code.

'm currently writing some rudimentary import/export automated tests

Great!

However I feel that is for an other patch as this one has grown quite a bit.

Lets get this patch out of the way before working on even more recent FFmpeg versions.

Does this look good to you guys

Some nit-picking. The most important is to test with video/audio decode/encode. Is this something you need help with, or you can/already did?

intern/ffmpeg/ffmpeg_compat.h
540	See the "Comment Sections" of https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
source/blender/blenkernel/intern/writeffmpeg.c
191	Maybe time to use `bool`? Although, can be done as a followup cleanup `int` -> `bool` in the module.
1083	I thought `BKE_report` will do prints as well. Any reason to duplicate error message without extra details.
1101–1102	Sometimes it's `ret` sometimes it's `err` ;)

In D10338#285587, @Sergey Sharybin (sergey) wrote:

The most important is to test with video/audio decode/encode. Is this something you need help with, or you can/already did?

I've done some quick test where I imported video files and exported them and looked if they were any noticeable differences.
It seems to work for me. However more people checking is also good :)

source/blender/blenkernel/intern/writeffmpeg.c
1083	I think I did this so the errors are also printed in the terminal. With this I could try to do some batch operators and only have to loop at the terminal output to see if things went wrong or not.
1101–1102	Good catch, I'll change `err` to `ret` as it is a return code and not exclusively an error code.

I'll do some tests tomorrow morning.

The code seems fine, and some quick tests I did here did work as well.
I think most efficient is to get it to the master branch and keep an eye on possible reports from artists.

The err vs. ret rename can happen without re-iterating on the patch.

@Richard Antalik (ISS), did you test the latest version of this patch?

This revision is now accepted and ready to land.May 7 2021, 10:11 AM

In D10338#285816, @Sergey Sharybin (sergey) wrote:

@Richard Antalik (ISS), did you test the latest version of this patch?

I haven't. Will test now.

All seems to work fine here as well.

Closed by commit rB8d6264ea12bf: Cleanup: Remove deprecated variables and functions calls from our ffmpeg code (authored by Sebastian Parborg (zeddb)). · Explain WhyMay 7 2021, 5:12 PM

This revision was automatically updated to reflect the committed changes.

Sebastian Parborg (zeddb) added a commit: rB8d6264ea12bf: Cleanup: Remove deprecated variables and functions calls from our ffmpeg code.

Jeroen Bakker (jbakker) added a commit: rBf522e8db2288: Cleanup: Remove deprecated variables and functions calls from our ffmpeg code.Jun 30 2021, 9:48 AM

Richard Antalik (ISS) mentioned this in T91405: VSE: Block artefacts in WEBM video on preview or export.Nov 2 2021, 7:13 AM

Cleanup: Remove deprecated variables and functions calls from our ffmpeg code
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 34824

CMakeLists.txt

intern/ffmpeg/ffmpeg_compat.h

source/blender/blenkernel/intern/writeffmpeg.c

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

source/blender/imbuf/intern/util.c

Cleanup: Remove deprecated variables and functions calls from our ffmpeg codeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 34824

CMakeLists.txt

intern/ffmpeg/ffmpeg_compat.h

source/blender/blenkernel/intern/writeffmpeg.c

source/blender/imbuf/intern/anim_movie.c

source/blender/imbuf/intern/indexer.c

source/blender/imbuf/intern/util.c

Cleanup: Remove deprecated variables and functions calls from our ffmpeg code
ClosedPublic
Actions

Revision Contents
Changeset List