Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update FFmpeg to version 7.1 #950

Open
wants to merge 76 commits into
base: master
Choose a base branch
from

Conversation

ulmus-scott
Copy link
Contributor

Checklist

Commits

The commits in order are:

FFmpeg reversions
#923

Eliminate usage of internal FFmpeg headers
#931

mpegts-mythtv harmonization
#927

Changes to make MythTV compile with FFmpeg 7.1:

6716364c61 cmake/configure: remove references to unsupported crystalhd
d271e34a86 constify AVIOContext::write_packet buf parameter
1304aee821 constify AVIOContext::write_packet buf parameter (plugins)
84c178159a Use AVFrame::best_effort_timestamp

Update to FFmpeg 7.1:

976ab94b53 update FFmpeg to version 7.1
f8928e842c add ffmpeg_update_instructions.md

Cleanup after update:

807ef793aa remove support for LIBAVFORMAT_VERSION_MAJOR < 61 (plugins)
6c4c2684e2 remove support for LIBAVFORMAT_VERSION_MAJOR < 61
5098f349fb fix TestMusicMetadata::test_mp4()

Silence new FFmpeg deprecation warnings:

383aca2ea1 libmythtv/io/mythavformatwriter.cpp: silence AVCodec deprecation warnings
d43d574a59 replace deprecated av_stream_get_side_data()
93a9bc4d3c replace deprecated av_fft API with av_tx
ae3825083a replace deprecated AVFrame::interlaced_frame, top_field_first, and key_frame
7849f3029b mytharchivehelper.cpp: replace deprecated AVFrame::key_frame
e4e850b5e7 replace deprecated av_fft API with av_tx (mythmusic)

Other cleanup commits made while investigating:

dc03e8d08b remove DecoderBase::m_getRawFrames and m_getRawVideo
8f08f60856 remove unused no-op functions from DecoderBase
ca0e299060 mythaverror: add new functions returning std::string
b22e19e8b9 split libmyth/mythavframe.h from libmyth/mythaverror.h
7fe6c88070 create AvFormatDecoder::do_av_seek()
31af3ce5e2 remove unused GetAVTimeBaseQ()
552e90b564 MythMediaBuffer::WaitForAvail(): simplify return logic

Testing

@kmdewaal , I have reverted your fix regarding blocking with VDPAU referencing https://trac.ffmpeg.org/ticket/9532. Could you test if it is still an issue and update the FFmpeg trac as needed?

I have tested with my various Video samples and US OTA ATSC recordings and everything appeared to work the same. I do not have any hardware acceleration set up, so I could not test with that.

Testing with hardware acceleration or other source types would be helpful.

Quirks in master I may look into more

AvFormatDecoder::GetFrame() log spam every ~100μs: decoding error End of file if paused near end of file. I can sometimes trigger this with Video files when skipping by 1 second near the end while paused.

The seeking code for Videos is very fragile, I suspect multithreading issues.
Sometimes either seek forward or backward does nothing and gives the following error in the log:
AFD: av_seek_frame(ic, -1, 13449820978, 0) -- error
Some further investigation revealed the error code to be -1, which FFmpeg thinks
is EPERM.

For a short (~16s) audio only file I created, skipping backwards usually causes playback to exit instead of skipping.

This reverts commit 049fabc.

Reduce changes to FFmpeg.
Instead of the internal FFmpeg header compat/cuda/dynlink_loader.h.  This leaves
only one internal FFmpeg header used by MythTV.

Per the comment on win32_dlopen() in FFmpeg/compat/w32dlfcn.h, Windows will now
additionally search the current directory for nvcuda.dll and nvcuvid.dll, which
FFmpeg considers less secure.

This is an atomic block and the order must be preserved:
 #include "libavutil/log.h"
 #define FFNV_LOG_FUNC(logctx, msg, ...) av_log(logctx, AV_LOG_ERROR, msg,  __VA_ARGS__)
 #define FFNV_DEBUG_LOG_FUNC(logctx, msg, ...) av_log(logctx, AV_LOG_DEBUG, msg,  __VA_ARGS__)
 #include <ffnvcodec/dynlink_loader.h>

 #include "libavutil/hwcontext_cuda.h"
must come after
 #include <ffnvcodec/dynlink_cuda.h>
which is also transitively included by #include <ffnvcodec/dynlink_loader.h>
This reverts commit 5f9b27c.

MythTV no longer uses that file, so remove the modification.
…h (part 1)

Since we don't need the full buffer, we could use avio_read_partial() instead.
…h (part 3)

I don't know if any of this is necessary since FFmpeg's avio API is already
buffered.  However, I did not investigate using avio directly with avio_open()
nor did I investigate what our buffer actually does.

I'm not sure why MythAVFormatBuffer::Seek() with Whence = SEEK_END did not ever
actually seek.

libavformat/avio.h says this about read_packet:
"
A function for refilling the buffer, may be NULL.
For stream protocols, must never return 0 but rather
a proper AVERROR code.
"

So I'm not sure the first return of 0 was correct.  AVERROR(EINVAL) is a more
useful return value for all of the functions, instead of 0.
The only STREAM_TYPE_* define used is STREAM_TYPE_PRIVATE_DATA.

STREAM_TYPE_AUDIO_DTS is already defined by FFmpeg to a different value.  0x8a
is used in MISC_types[].

STREAM_TYPE_AUDIO_HDMV_* defines are from:
MythTV@d15e482
They are now unused and FFmpeg has added the entries to HDMV_types[].
STREAM_TYPE_SUBTITLE_DVB is from:
MythTV@aaa9372#diff-ae852538fd61b663231c8bdda1fdbade40c97e8305401fcc6f1ad9f395efdf18
STREAM_TYPE_VBI_DVB is from:
MythTV@6da1854

However, the corresponding uses in MISC_types[] can never be used.
in mpegts_set_stream_info()
    pes->stream_type = stream_type;
...
        mpegts_find_stream_type(st, pes->stream_type, MISC_types);

mpegts_set_stream_info() is called either with stream_type = 0 or
stream_type = get8(&p, p_end)

0x100 and 0x101 exceed UINT8_MAX and thus will never be found in the search.
Use the names given in ETSI 300 468 and list all of the values used,
even if the defines aren't.
I'm not sure that the new name is correct since I don't have a copy of
ISO/IEC 13818-6.  However, it should be close.
Appears to have been from an FFmpeg merge in 2009:
MythTV@6d62e66
…ptor

This has no functional change since MythTV only uses the MythTV addition
AVStream::carousel_id if AVStream::codecpar::codec_id == AV_CODEC_ID_DSMCC_B.
originally from:
MythTV@a1d4d11
referencing:
https://code.mythtv.org/trac/ticket/1887

ISO/IEC 13818-1:2021 specifies a valid range of [0x0010, 0x1FFE] in
§ 2.4.4.6 Semantic definition of fields in program association section
and Table 2-3 – PID table
…ng, and pmt_scan_state

req_sid is from an FFmpeg merge:
MythTV@2a56a37#diff-2fb8a33ba832f242d27f19c9f25755b57af9807343e99dac77263c1c5ebc383a
removed from FFmpeg in
FFmpeg/FFmpeg@90d13e3

scanning and pmt_scan_state are from
MythTV@084e3f8

MpegTSContext::req_sid is only set by mpegts_read_header().

First, in pmt_cb() always create PMT filters.
Now we don't want to stop_parse early since that would prevent creating all the
PMT filters, so remove those changes to pat_cb() and pmt_cb().

MpegTSContext::pmt_scan_state is only tested in mpegts_read_header() and its use
is equivalent to `!ts->prg[i].pmt_found`.

Now MpegTSContext::scanning and MpegTSContext::pmt_scan_state are unused, so
remove them.

MpegTSContext::req_sid is now set but otherwise unused, so remove it as well.

Additionally, in mpegts_read_header() sid would cause an invalid index of -1 on
the first iteration.

The fallback code in mpegts_read_header() has been broken since
MythTV@0b0068c

sid != pmt_pid, so it was looking for the wrong filter in MpegTSContext::pids.
Disabling CRC is from:
MythTV@084e3f8
referencing
https://code.mythtv.org/trac/ticket/328

However, the sample does not trigger that fallback.

Deal with incomplete PMT streams in BBC iPlayer IPTV
MythTV@c11ee69
references
https://code.mythtv.org/trac/ticket/9926

The sample is unavailable and the issue was not in FFmpeg's unmodified demuxer,
so, following subsequent harmonization, I assume it is no longer an issue.  If
it still is, MythTV's other modifications should be looked at first.
There is one PMT per program, so add the stream_type to the corresponding
Program.  This means we no longer need to look at an MpegTSFilter.  The
searching logic in is_pmt_equal() is equivalent to that in pmt_equal_streams().
However, we now look at the entire PMT not just is_desired_stream()s.

Nothing now uses MpegTSContext::pid_cnt or MpegTSContext::pmt_pids, so remove
them and a lot of other now unused code.

See ISO/IEC 13818-1:2021 § 2.4.4.9 Program map table
Perhaps we could use TS_program_map_section()'s version_number and possibly
current_next_indicator instead of comparing the PMT ourselves?  Maybe by
modifying update_av_program_info()?

export_pmt() would probably need
if (old_version == -1 || old_version != version)
also its buffer should probably be moved to AVProgram since each program has a
different PMT.

Similarly streams_changed() should have a program_number parameter, since we
probably only care about one program at a time.
This can be a problem when there is more than one program in one file, e.g.
a full TS capture with multiple digital subchannels.

The `#if`ed out log in AutoSelectAudioTrack was broken; I replaced it with
something that should be similar.
They were only used by MythTranscodePlayer so move them there.
UpdateStoredFrameNum(), WriteStoredData(), ClearStoredData()
and fix Doxygen syntax for other functions.
and eliminate unnecessary transitive includes of those files.
The first return ensures (available < count) is always true.

If m_ateof is true, then Count = available, which means (available < count) is
false and all subsequent conditionals are false.

Now, (available < count) is only needed for the while loop condition.
FFmpeg removed the used function and the recommendation was to use
AVCodecContext.hw_frames_ctx or hw_device_ctx instead.  However,
the code is already using those, so I don't know what it was supposed
to be doing.
@warpme
Copy link
Contributor

warpme commented Oct 24, 2024

Scott,
Per our conversation on ML about ffmpeg5->ffmpeg7 regression on n3450 with 2x vaapi deinterlacer: pls find 2 logs for playback of interlaced h264 recording. ffmpeg5 has working 2x DI while ffmpeg7 reports on osd "2x VAAPI Compensated" but visually i see 1x DI on screen
I was trying to catch diff between logs but failed to see meaningful diff :-(
n3450-ffmpeg7-nok.log.zip
n3450-ffmpeg5-ok.log.zip

@ulmus-scott
Copy link
Contributor Author

@warpme I had made a typo in mythvaapiinterop.cpp which is fixed in the first new commit and hopefully fixes the double rate VAAPI deinterlacing.

I did not see anything of interest in the logs either.

If the new commit doesn't work, try testing at 93a9bc4 and, if that still doesn't work, at 84c1781 before the FFmpeg update.

Before merging I want to move some of the final commits earlier and squash some of them together.

@warpme
Copy link
Contributor

warpme commented Oct 26, 2024

Scott,

Current head works perfectly now!
Many thx fixing this.
I will start to use ffmpeg7.1 branch in production on x86 and aarch64 systems to see how stability, hw decoding for various codecs looks like.
I'll put report on dev ML to inform also others how well ffmpeg7.1 works

This reverts commit db3dfa505a2a5a7e2b6ff898f9c3ad5d74fbf01a.

I am not sure the "optimization" was correct and removing the customization
appears to have no effect of any kind.  My MHEG sample did not trigger the log
message for "All DSM info found".
The code in libavcodec/mpeg12dec.c will never create a cc_type of 2 for SCTE-20
and cc_type is guaranteed to be less than 2 otherwise by the if condition.

AvFormatDecoder::m_invertScteField is now always 0, so remove it.

AvFormatDecoder::m_lastScteField is now set but unused, so remove it.
…captions

see mpeg_decode_a53_cc()

Differences:
FFmpeg does not have the initial two bytes that MythTV added to signal valid and
length.

FFmpeg will only use the first type of embedded CC that it sees, whereas MythTV
would switch between ATSC and SCTE-20 under certain conditions.  If multiple
types were present MythTV would concatenate them.

FFmpeg allows forcing the subtitle format with the cc_format option in mpeg12dec.c.

DVD CC:
FFmpeg does not check p[4]

ATSC A/53:
FFmpeg checks the process_cc_data_flag in p[5].

SCTE-20:
FFmpeg ignores line_offset, which MythTV ensures is 11 for line 21 data.

FFmpeg does not set one_bit or the 4 reserved bits before cc_valid and cc_type
(cc608hdr vs cap[0]), but these are only to avoid emulating start codes and are
not checked, so it doesn't matter.

p[0] == 0x05 && p[1] == 0x02:
Moved to mpeg_decode_a53_cc().

Originally from:
Fixes #2481, by applying patch from kenny at the-b org. This adds dec…
MythTV@49d3294

Referencing:
https://code.mythtv.org/trac/ticket/2481

Modified in:
Simplify DTV CEA-608 handling a bit & add some range checking.
MythTV@be6a2b4

Some tweaks to cc implementation in mpeg12.c: make the C ANSI compati…
MythTV@77cff32

See also:
https://github.com/CCExtractor/ccextractor/blob/master/src/lib_ccx/es_userdata.c

MythTV:
SCTE-20 encapsulates CTA-608, which is where XDS is defined, so I don't know why
the condition was specific to SCTE-20.
This reverts commit 641109a.

David Hampton confirmed on the mailing list that this change is no longer
necessary on FreeBSD 13/14/15.
…Android)'

Neither header is checked by FFmpeg's configure and neither HAVE_EGL_EGL_H
nor HAVE_GLES2_GL2_H is defined.  FFmpeg/configure has not tested those since
MythTV@018be3f
in 2018.

FFmpeg's configure never tested eglGetProcAddress and thus would never define
HAVE_EGLGETPROCADDRESS.

MythTV's configure does check them and (myth)config.h does define HAVE_EGL_EGL_H,
HAVE_GLES2_GL2_H, and HAVE_EGLGETPROCADDRESS; however, FFmpeg does not use
mythconfig.h, using its own config.h instead, probably since the decoupling of
MythTV and FFmpeg's configure in
MythTV@9563786
in 2018.
by merging the code into AvFormatDecoder::RemoveAudioStreams() and replacing
ff_flush_packet_queue() with avformat_flush().

The original code could have accessed out of bounds if the first stream was an
audio stream, since i would have been -1 on the next iteration.  This would also
cause it to recheck previously checked streams when a stream was removed.

By setting the index first, we don't have to iterate twice to renumber the streams.

Slightly more is reset by avformat_flush(), see libavformat/seek.c, compared to
just ff_flush_packet_queue(), but it is a public function and resetting the
extra values is probably better since it more fully resets the AVFormatContext.
Additionally, the AVFormatContext is now only reset once instead of after
removing each stream.
StreamInfo::m_av_substream_index is only used for kTrackTypeAudio.  It is no
longer set to 0 by:
mythdvddecoder.cpp
AvFormatDecoder::ScanStreams() for AVMEDIA_TYPE_SUBTITLE

StreamInfo::m_orig_num_channels is only used by AvFormatDecoder::GetTrackDesc()
to print the number of audio channels for kAudioTypeNormal.
The following no longer set m_orig_num_channels to 0:
mythdvddecoder.cpp
cc708reader.cpp
decoderbase.cpp
avformatdecoder.cpp

StreamInfo::m_easy_reader and m_wide_aspect_ratio were unused but set by:
AvFormatDecoder::ScanATSCCaptionStreams() (incorrectly, setting
m_orig_num_channels = easy_reader and m_easy_reader = wide_aspect_ratio)

AvFormatDecoder::ScanStreams() for AVMEDIA_TYPE_AUDIO (appeared to set m_easy_reader
in an unused attempt to flag the second StreamInfo for dual mono audio)

decoderbase.cpp: I don't know why one of the default CC608 set m_language_index
to 2, but it is not used for closed captions.
If continue is DVD specific and from
MythTV@8bcef59

AVStream::id will never be negative for MPEG-TS or MPEG-PS.
It was a copy of a value from AVCodecParameters and was only used for printing.

If the channel count from FFmpeg is wrong, add this to GetTrackDesc() to get the
value from libdvdnav:

if (m_ringBuffer->IsDVD())
    channels = m_ringBuffer->DVD()->GetNumAudioChannels(m_tracks[kTrackTypeAudio][TrackNo].m_stream_id);
When attempting to upstream the change before, it was recommended to instead
use AV_CHANNEL_ORDER_CUSTOM with two AV_CHAN_FRONT_CENTER channels. See
https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/

audiotoolboxdec.c will no longer flag dual_mono since it uses
AV_CHANNEL_ORDER_UNSPEC.

SetupAudioStreamSubIndexes() will create or destroy the second StreamInfo for
the second substream, leaving the duplicate StreamInfo from ScanStreams()
untouched, allowing playing both channels as stereo.
st and st->codecpar will already have been dereferenced by then.
Originally from:
MythTV@6aaf617

references:
https://code.mythtv.org/trac/ticket/5978
I could not find the referenced FFmpeg bugs from the comment or MythTV trac
since FFmpeg has changed its bug tracker. However, I did find
https://samples.ffmpeg.org/ffmpeg-bugs/roundup/issue378/Issue378/
which plays back with subtitles fine.

As far as I can tell the change reduces out_size by len + 6 if that section is
at the end of p.  However, that data is all in the buffer, so I don't see how
that would fix anything.  If this does actually fix something a new FFmpeg bug
report should be created with a sample.
libavcodec/codec_desc.c: move to correct location and correct description
libavcodec/codec_id.h: move to correct location in data codecs
libavformat/mpegts-mythtv.c: reuse teletext_descriptor code for VBI_teletext_descriptor

libmythbase/iso639.h: add used include
libmythbase/stringutil.h: add new funtion split_sv()
libmythtv/decoders/avformatdecoder.cpp:
AvFormatDecoder::ScanTeletextCaptions() was specific to AV_CODEC_ID_DVB_TELETEXT,
looking only at the teletext_descriptor in the PMT.

ScanTeletextCaptions() no longer uses the MythTV exported PMT, using what FFmpeg
already exported when it parsed PMTs.  Both AV_CODEC_ID_DVB_TELETEXT and
AV_CODEC_ID_DVB_VBI (if there is EBU teletext data in the VBI data) now add to
the tracks list and multiple streams are now supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants