Add support for FFmpeg 5.0 #910

jlaine · 2022-03-15T16:25:13Z

No description provided.

jlaine · 2022-03-15T23:01:48Z

@mikeboers This is progressing better than I thought. One interesting aspect is that we now hit the exact same failures as the ones encountered against FFmpeg 4.4.

uvjustin · 2022-03-22T08:34:06Z

@jlaine Using a separate AVCodecContext for PyAV is the same approach I took when I attempted a refactor. I thought there would be more issues with PyAV using a AVCodecContext distinct from the internal one held by AVStream, but it doesn't seem like this causes too many problems.
It does seem like framerate in AVCodecContext is not passed through AVCodecParameters when copying back and forth, and there seem to be a few different framerate/time_base fields between AVCodecContext and AVStream, and I think we need to take care to maintain consistency there.
BTW, the subtitles test issue with ffmpeg 5.0 seems to be related to the deprecation of the old ASS format with timing here: https://ffmpeg.org/pipermail/ffmpeg-cvslog/2016-February/098534.html .

uvjustin · 2022-03-22T08:45:45Z

av/container/output.pyx

+            to_avrational(rate or 24, &codec_context.framerate)

            stream.avg_frame_rate = codec_context.framerate
            stream.time_base = codec_context.time_base


I realize this was in the existing code, but is codec_context.time_base ever set before here?

I think time_base gets initialized to a sane value, but it would be worth double checking. I really appreciate you looking at this PR, feel free to tinker and suggest changes as I'm not 100% sure of what I'm doing! When in doubt I look at how FFmpeg (CLI) does it.

Sure, I will also tinker with it a bit when I get a chance. It doesn't seem too far off. I think some of the failing tests may actually be the result of existing mismatches between PyAV and ffmpeg. It's hard to get everything to align fully as some of the fields like framerate can get set by ffmpeg in a number of different places depending on the container/codec used.

jlaine · 2022-03-22T10:28:17Z

NOTE: I noticed there is an issue in close_output, because Container.streams can already be partially destroyed, so calling it from OutputContainer.__dealloc__ is not reliable. I'm not sure I can do much than emitting a warning if this happens.

I've probably also introduced a memory leak, I need to free the AVCodecContext. EDIT: no I did not, as the AVCodecContext is wrapped in a Python CodecContext which frees the C structure.

uvjustin · 2022-03-24T12:57:12Z

For the subtitles test failing on 5.0, I think we can just change the test to also accept the regular ASS format without the "Dialogue: " prefix and the two timestamps (basically just allow what it's failing on now).

jlaine · 2022-03-24T13:41:49Z

For the subtitles test failing on 5.0, I think we can just change the test to also accept the regular ASS format without the "Dialogue: " prefix and the two timestamps (basically just allow what it's failing on now).

No I don't think that's the issue. I've tried changing the format, which does strip away the timings from the output. However I think it masks the underlying issue which is that the timings look plain wrong.

uvjustin · 2022-03-24T14:24:20Z

The current ASS format doesn't include the timings in the output though. Are you checking them in SubtitleProxy?
Edit:
If you are referring to the 0.97 and 2.54 subtitle times embedded in the ASS string, those are only in the deprecated ASS format, but we can check them by adding:

        self.assertIsInstance(subs[0], SubtitleSet)
        self.assertEqual(subs[0].pts, 970000)
        self.assertEqual(subs[0].start_display_time, 0)
        self.assertEqual(subs[0].end_display_time, 1570)

pts is in lib.AV_TIME_BASE and start/end display times are in ms, we have:
start == subs[0].pts / lib.AV_TIME_BASE + subs[0].start_display_time / 1000 == 0.97
end == subs[0].pts / lib.AV_TIME_BASE + subs[0].end_display_time / 1000 == 2.54

jlaine · 2022-03-27T16:55:34Z

The current ASS format doesn't include the timings in the output though. Are you checking them in SubtitleProxy? Edit: If you are referring to the 0.97 and 2.54 subtitle times embedded in the ASS string, those are only in the deprecated ASS format, but we can check them by adding:
        self.assertIsInstance(subs[0], SubtitleSet)
        self.assertEqual(subs[0].pts, 970000)
        self.assertEqual(subs[0].start_display_time, 0)
        self.assertEqual(subs[0].end_display_time, 1570)

OK you're exactly right, so I have reworked the tests as suggested to explicitly check timings. I've also made "ass" format the default instead of "ass_with_timings" to get consistent behaviour across FFmpeg versions.

That leaves the other failures related to rates.

uvjustin · 2022-06-17T12:16:08Z

@jlaine This looks pretty good and the tests seem to be passing.
I'll make a build against ffmpeg 5.0 and test it against our usage in Home Assistant.

jlaine · 2022-06-17T14:13:03Z

I had put off merging this to see if any bug-fixes needed to land in the 9.x branch, as it will be the last one supporting FFmpeg < 4.3. It looks as though we're in a good place, so we're close to the point where this PR should land.

One thing that bothers me is that we're hardly testing anything at all on Windows (i.e. only FFmpeg 4.3). Conda seems to have dropped the ball on packaging FFmpeg for Windows..

uvjustin · 2022-06-17T17:48:28Z

Maybe we can try to use the Windows binaries from https://github.com/BtbN/FFmpeg-Builds/releases instead of from Conda?

uvjustin

Looks pretty good. I left some comments with a few things I noticed when getting a stopgap release ready on the ha-av fork.

av/container/output.pyx

uvjustin · 2022-06-22T07:28:54Z

av/container/input.pyx

+            if codec:
+                # allocate and initialise decoder
+                codec_context = lib.avcodec_alloc_context3(codec)
+                err_check(lib.avcodec_parameters_to_context(codec_context, stream.codecpar))


avcodec_parameters_to_context does not set the framerate field of AVCodecContext: https://www.ffmpeg.org/doxygen/trunk/codec__par_8c_source.html#l00182
As Stream.rate/Stream.framerate relies on getattr pass through to its codec_context's rate/framerate, this means Stream.rate/Stream.framerate are no longer valid for input video streams.
Two ways we can remedy this:

We can set the codec_context's framerate here and continue to rely on this passthrough feature.

We can add rate/framerate properties directly to VideoStream.
Note that AudioStream does not have this issue as the sample_rate field gets copied properly.

I'm not even sure we should be doing anything:

https://www.ffmpeg.org/doxygen/trunk/structAVCodecContext.html#a4d08b297e97eefd66c714df4fff493c8

decoding: For codecs that store a framerate value in the compressed bitstream, the decoder may export it here. { 0, 1} when unknown.

I'm pretty sure it's not always set. It was causing a failure in our test suite that I remedied using the 2nd option above.
However, what you said actually implies we should rely on the first option above (passthrough), because the decoder may export it when decoding the bitstream.

I think we've reached the point where not merging is actively hurting the project, so I'm going to hit the button. Would you mind opening an issue to discuss the "framerate" point you made?

.github/workflows/tests.yml

We now allocate and populate an AVCodecContext ourselves. avcodec_copy_context is also gone, so stop using it. We relax the Stream.average_rate tests for older FFmpeg, as the videos output by these older FFmpeg's seem to give a slightly wrong FPS since the switch to our own AVCodecContext.

jlaine marked this pull request as draft March 15, 2022 16:25

jlaine force-pushed the no-stream-codeccontext branch from f1e581e to 71478e3 Compare March 15, 2022 20:45

jlaine mentioned this pull request Mar 15, 2022

PyAV does not build with ffmpeg 5.x #817

Closed

6 tasks

jlaine force-pushed the no-stream-codeccontext branch from 71478e3 to a611f51 Compare March 15, 2022 22:54

jlaine force-pushed the no-stream-codeccontext branch from a611f51 to 7c6375a Compare March 15, 2022 23:17

jlaine changed the title ~~[streams] stop using deprecated Stream.codec, it's gone in FFmpeg 5~~ Add support for FFmpeg 5.0 Mar 15, 2022

jlaine force-pushed the no-stream-codeccontext branch 4 times, most recently from 0673d00 to 5c954d8 Compare March 22, 2022 07:41

uvjustin reviewed Mar 22, 2022

View reviewed changes

jlaine force-pushed the no-stream-codeccontext branch from 5c954d8 to 6b1295f Compare March 24, 2022 07:17

jlaine mentioned this pull request Mar 25, 2022

Can't use stream added from template for anything other than remuxing #730

Closed

6 tasks

jlaine force-pushed the no-stream-codeccontext branch 2 times, most recently from ae1619c to 5eaa500 Compare March 27, 2022 15:17

jlaine force-pushed the no-stream-codeccontext branch 6 times, most recently from 557cc2b to 6c70bf7 Compare April 1, 2022 08:25

jlaine marked this pull request as ready for review April 1, 2022 08:26

jlaine force-pushed the no-stream-codeccontext branch from 6c70bf7 to 5695e7e Compare April 1, 2022 11:19

jlaine force-pushed the no-stream-codeccontext branch 2 times, most recently from 23fd0dc to e075808 Compare April 1, 2022 13:20

hmaarrfk mentioned this pull request Apr 10, 2022

Stop ffmpeg migration to 5.0 conda-forge/conda-forge-pinning-feedstock#2754

Merged

5 tasks

jlaine force-pushed the no-stream-codeccontext branch 2 times, most recently from 53d8da3 to 428e50f Compare April 20, 2022 14:59

WyattBlue mentioned this pull request May 30, 2022

Error install pyav on newest ffmpeg! #988

Closed

uvjustin suggested changes Jun 22, 2022

View reviewed changes

jlaine added 2 commits June 25, 2022 16:42

[tests] run tests against FFmpeg 5.0, drop 4.0 - 4.2

a6a832a

jlaine force-pushed the no-stream-codeccontext branch from 428e50f to c02ac56 Compare June 25, 2022 14:42

jlaine merged commit 1870465 into PyAV-Org:main Jul 6, 2022

This was referenced Jul 6, 2022

VideoStream.rate and VideoStream.framerate no longer available #1005

Closed

Add rate and framerate properties to VideoStream (fixes: #1005) #1007

Closed

uvjustin mentioned this pull request Oct 17, 2022

Remove extraneous rate attributes #1034

Closed

This was referenced Oct 30, 2022

Upgrade pyav dependency to 10.0.0 with ffmpeg5 support cvat-ai/cvat#5196

Closed

Update PyAV dependency to 10.0.0 with ffmpeg5 support cvat-ai/cvat#5200

Closed

Add support for FFmpeg 5.0 #910

Add support for FFmpeg 5.0 #910

Uh oh!

Conversation

jlaine commented Mar 15, 2022

Uh oh!

jlaine commented Mar 15, 2022

Uh oh!

uvjustin commented Mar 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlaine commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uvjustin commented Mar 24, 2022

Uh oh!

jlaine commented Mar 24, 2022

Uh oh!

uvjustin commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlaine commented Mar 27, 2022

Uh oh!

uvjustin commented Jun 17, 2022

Uh oh!

jlaine commented Jun 17, 2022

Uh oh!

uvjustin commented Jun 17, 2022

Uh oh!

uvjustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jlaine commented Mar 22, 2022 •

edited

Loading

uvjustin commented Mar 24, 2022 •

edited

Loading