FFmpegWriter: Macro & member cleanup, to build with newer releases

[ferdnyc]

This should fix at least part of the issues currently causing our CI builds to fail. It eliminates a member pointer in openshot::FFmpegWriter that didn't need to be there (and was being abused while it was), and it vastly simplifies the ugliest macro in all of FFmpegUtilities.h.

• The fmt class member, which was of type AVFormat*, was really
  just an unnecessary copy of (AVFormatContext*)oc->oformat.
  But we were ASSIGNING into its members, which we were definitely
  not supposed to be doing. (And in recent FFmpegs, now that
  AVFormat* has been constd, we can't.) It's gone; now we just
  use oc->oformat anywhere we used to access fmt.

• The preprocessor macro to allocate a new stream was a mess of
  cross purposes: It did allocate a stream, but then it also
  allocated a new AvCodecCtx on newer FFmpeg releases. Worse (and
  always galling to me), it proceeded to assign to a variable
  that WASN'T passed in to the macro, just taking it on faith that
  it would only be used where that variable was defined. That's
  just... ugh. So I broke it apart into two steps (stream creation
  and context allocation), realized the stream creation code was
  the same for all ffmpeg versions and didn't need to be a macro
  at all, and now a 4-parameter, 6-line magical macro has been
  replaced with a simple, zero-side-effect one-liner.

• I also cleaned up the add_video_stream() code to be more like
  the add_audio_stream() code, since they were bad-different for
  no discernible reason.

[codecov]

Codecov Report

Merging #809 (6c54640) into develop (f2b89e3) will decrease coverage by 2.05%.
The diff coverage is 57.14%.

@@             Coverage Diff             @@
##           develop     #809      +/-   ##
===========================================
- Coverage    50.83%   48.78%   -2.06%     
===========================================
  Files          161      185      +24     
  Lines        13454    18191    +4737     
===========================================
+ Hits          6840     8875    +2035     
- Misses        6614     9316    +2702     

Impacted Files	Coverage Δ
src/FFmpegUtilities.h	100.00% <ø> (ø)
src/FFmpegWriter.h	25.00% <ø> (ø)
src/FFmpegWriter.cpp	59.18% <57.14%> (-4.93%)	⬇️
src/effects/Crop.h	25.00% <0.00%> (-8.34%)	⬇️
src/Settings.cpp	95.00% <0.00%> (-5.00%)	⬇️
src/MagickUtilities.cpp	95.65% <0.00%> (-4.35%)	⬇️
src/Coordinate.cpp	96.29% <0.00%> (-3.71%)	⬇️
src/Color.cpp	81.81% <0.00%> (-2.40%)	⬇️
src/effects/Negate.cpp	30.23% <0.00%> (-2.20%)	⬇️
... and 120 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[ferdnyc]

This code now passes, but TBH I'm not 100% confident in it . The part of it I most had to screw with also happens to be the code I'm least familiar with, where it dumps raw frames to disk without using an encoder. I don't believe that code has any unit test coverage at all, really.

In fact, I started trying to write a unit test for it... but then realized I don't actually know HOW to steer FFmpegWriter down that code path. I've never been clear on exactly what conditions lead to an AVFMT_RAWPICTURE stream, or when that would come up in actual use.

Does anyone have any actual usage examples for the "bypassed-encoder" part of FFmpegWriter? @jonoomph ? @eisneinechse ? @jeffski ? (I have vague memories of you maybe mentioning that you were a consumer of that code, I think?)

I'd be happy to write a unit test for it, if someone can point me to how.

(Specifically, I'm referring to this code, where av_send_packet() and av_receive_frame() are bypassed in favor of dumping the frame's buffer data directly into a packet and serializing it straight to disk:)

libopenshot/src/FFmpegWriter.cpp

Lines 2125 to 2150 in f2b89e3

	if (oc->oformat->flags & AVFMT_RAWPICTURE) {
	#endif
	// Raw video case.
	AVPacket pkt;
	av_init_packet(&pkt);
	pkt.flags |= AV_PKT_FLAG_KEY;
	pkt.stream_index = video_st->index;
	pkt.data = (uint8_t *) frame_final->data;
	pkt.size = sizeof(AVPicture);
	// Set PTS (in frames and scaled to the codec's timebase)
	pkt.pts = video_timestamp;
	/* write the compressed frame in the media file */
	int error_code = av_interleaved_write_frame(oc, &pkt);
	if (error_code < 0) {
	ZmqLogger::Instance()->AppendDebugMethod(
	"FFmpegWriter::write_video_packet ERROR ["
	+ av_err2string(error_code) + "]",
	"error_code", error_code);
	return false;
	}
	// Deallocate packet
	AV_FREE_PACKET(&pkt);

(That sizeof(AVPicture) was the first thing that had to go, as AVPicture is no longer present in the latest FFmpeg releases at all. Finally!)

[ferdnyc]

This is removed because you can't assign to values in the codec — it's now consted, so even the deprecation-override pragma wouldn't cut it anymore. This was never necessary, whatever it was trying to achieve can't be done the way it was trying to do it. Modifying the AVFormat, the AVCodec, or any other ffmpeg structure that's not one of the corresponding contexts (AVFormatContext, AVCodecContext, etc.) has always been unsupported, and with the expanded const-correctness of the latest library versions, is no longer possible.

[ferdnyc]

I'm not convinced this is necessary — it should be automatic as part of the eventual avcodec_parameters_from_context() call — but I left it in anyway.

[ferdnyc]

@jonoomph @JacksonRG @eisneinechse @jeffski @AAAANYONE!

Our CI is failing, and has been for some time. This PR fixes the build, so even though I'm not confident all of the code is correct (see comments above), last call before I merge it just to get us building again. The only likely casualties are relatively obscure code paths dealing with raw video frames. (Which are also not unit tested, so there's no way for us to know if I broke anything except to just try it and see who complains.)

[eisneinechse]

@ferdnyc Most of that code was there before I joined and was only kept to keep the compatibility to older, way older, versions of ffmpeg.It would be great if we could do away with this relics and have a more readable source and have a source that passes the CI. I am for it if anyone cares.

[ferdnyc]

@eisneinechse Yeah, as I said elsewhere (see #784) all of the code that supports FFmpeg < 3.0 is, well, I'll just copy my comments from over there:

FFmpeg 2.8 — which we still depressingly support (in theory) — doesn't have an av_packet_alloc(), so its use is sequestered behind an #if IS_FFMPEG_3_2 check. If that fails the old av_init_packet() method is used instead, but it now initializes an AVPacket* instead. Hopefully that will work; if not, we don't have any CI that checks FFmpeg < 3.4 anymore, so I guess we'll find out if an Ubuntu 16.04 user complains. Of course, any remaining Ubuntu 16.04 users are running an unsupported system that's past its End-of-Life date, so I'm not really worried.

(This is my passive-aggressive way of saying, "We REALLY need to get all the crap FFmpeg 2.x code out of the library".) It's a whole lot of ugly code that may or may not still work — we have no idea, we haven't tested it in years.

FFmpeg 3.0 ushered in the era of the packet-centric pipeline, which radically changed the flow of code written with the library. Changes since then have been relatively minor, mostly deprecating or swapping around individual function calls in ways that don't change the program flow. But trying to keep FFmpeg 2.x and 3.x+ code together in the same codebase means loooong stretches of #ifdef'd code that do completely different things, while trying to pretend they're the same.

There is light at the end of the tunnel, though. JUCE 6.x only supports building with CMake 3.15+, which is way below the version available on FFmpeg 2.x platforms. So a JUCE upgrade will force us to leave those platforms behind whether we want to or not, making their abandonment a much easier sell. (I should really get that upgrade PR in, I suppose.)

If we're going to make CMake 3.15 our minimum required version to build the libraries, then we can easily set FFmpeg 3.4 as the minimum required dependency and dump not only all of the 2.x code, but also the 3.0 and 3.2 code paths. That'd let us pare down the macro hell of FFmpegUtilities.h to just two paths: pre-4.0 and post-4.0. Each set would also be much smaller, as there are relatively few differences between the APIs. (We could also dump all of the avresample-vs-swresample alternation, and just write the code to use swresample exclusively.)

[ferdnyc]

Well, I'mma go ahead and merge this one, as well as #784 after I do the inevitable conflict-resolution. That will also get rid of the warnings about av_packet_init() that even FFmpeg 4.x builds are spewing. Hopefully nothing will break, if it does we'll fix it. It'll hopefully be only some very old or very rarely-used code paths that suffer, since our unit tests should cover at least the most common uses.