1962139 - Support FFmpeg 8.0 (libavcodec 62) on Unix

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[felix-mozilla]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:138.0) Gecko/20100101 Firefox/138.0

Steps to reproduce:

Updated ffmpeg to the current git HEAD.

Actual results:

libavcodec.so.61 is gone, and it its place is libavcodec.so.62.
Firefox can't find it under this new name.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

We support latest stable release (7.1.1). Feel free to submit patch to support the HEAD, how-to is here:
https://mastransky.wordpress.com/2023/07/04/no-one-fights-alone-a-guide-to-your-first-firefox-patch-on-linux/
Thanks.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Rahul Chandra]

To add to this bug, Firefox does not support the newer version of AVCodec from FFMPEG 8.0+ (the stable release). The headers are not in place and there are some profile issues during the build from the renaming/refactoring in upstream FFMPEG,

Here is a snippet from the Linuxfromscratch bug tracker provided by Joe Locash (currently private)

Crude support for the headers was added with the below commands on a system running FFMPEG 8.0

sed -e '/ffmpeg61/a\ "ffmpeg62",' -i dom/media/platforms/ffmpeg/moz.build
mkdir -p dom/media/platforms/ffmpeg/ffmpeg62/include
cp dom/media/platforms/ffmpeg/ffmpeg61/moz.build dom/media/platforms/ffmpeg/ffmpeg62/
cp -a /usr/include/libav{codec,util} dom/media/platforms/ffmpeg/ffmpeg62/

And the following errors showed up during the build

14:19.41 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1208:38: error: no member named 'pkt_pos' in 'AVFrame'
14:19.41  1208 |         rv = CreateImageV4L2(mFrame->pkt_pos, GetFramePts(mFrame),
14:19.41       |                              ~~~~~~  ^
14:19.41 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1211:39: error: no member named 'pkt_pos' in 'AVFrame'
14:19.41  1211 |         rv = CreateImageVAAPI(mFrame->pkt_pos, GetFramePts(mFrame),
14:19.41       |                               ~~~~~~  ^
14:19.41 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1233:32: error: no member named 'pkt_pos' in 'AVFrame'
14:19.41  1233 |       rv = CreateImage(mFrame->pkt_pos, GetFramePts(mFrame), Duration(mFrame),
14:19.41       |                        ~~~~~~  ^
14:19.44 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1578:19: error: no member named 'key_frame' in 'AVFrame'
14:19.44  1578 |         !!mFrame->key_frame, TimeUnit::FromMicroseconds(-1));
14:19.44       |           ~~~~~~  ^
14:19.45 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1605:23: error: no member named 'key_frame' in 'AVFrame'
14:19.45  1605 |             !!mFrame->key_frame, TimeUnit::FromMicroseconds(-1));
14:19.45       |               ~~~~~~  ^
14:19.45 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1618:65: error: no member named 'key_frame' in 'AVFrame'
14:19.46  1618 |             TimeUnit::FromMicroseconds(aDuration), b, !!mFrame->key_frame,
14:19.46       |                                                         ~~~~~~  ^
14:19.46 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1691:17: error: no member named 'key_frame' in 'AVFrame'
14:19.47  1691 |       !!mFrame->key_frame, TimeUnit::FromMicroseconds(-1));
14:19.47       |         ~~~~~~  ^
14:19.47 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1740:17: error: no member named 'key_frame' in 'AVFrame'
14:19.47  1740 |       !!mFrame->key_frame, TimeUnit::FromMicroseconds(-1));
14:19.47       |         ~~~~~~  ^
14:20.74 In file included from Unified_cpp_ffmpeg_ffmpeg620.cpp:65:
14:20.74 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp:122:6: error: use of undeclared identifier 'FF_PROFILE_H264_BASELINE'
14:20.74   122 |     {FF_PROFILE_H264_BASELINE, "baseline"_ns},
14:20.74       |      ^~~~~~~~~~~~~~~~~~~~~~~~
14:20.90 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp:123:6: error: use of undeclared identifier 'FF_PROFILE_H264_MAIN'
14:20.90   123 |     {FF_PROFILE_H264_MAIN, "main"_ns},
14:20.90       |      ^~~~~~~~~~~~~~~~~~~~
14:21.06 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp:124:6: error: use of undeclared identifier 'FF_PROFILE_H264_EXTENDED'
14:21.06   124 |     {FF_PROFILE_H264_EXTENDED, ""_ns},
14:21.06       |      ^~~~~~~~~~~~~~~~~~~~~~~~
14:21.21 /tmp/build/firefox-140.2.0/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp:125:6: error: use of undeclared identifier 'FF_PROFILE_H264_HIGH'
14:21.21   125 |     {FF_PROFILE_H264_HIGH, "high"_ns}};
14:21.21       |      ^~~~~~~~~~~~~~~~~~~~

Comment 5

[Xi Ruoyao]

The FF_PROFILE_* things seem trivial, they are just renamed to AV_PROFILE_*. But I cannot see an obvious replacement for pkt_pos and key_frame.

Comment 6

[Xi Ruoyao]

Well, perhaps !!mFrame->key_frame should be just !!(mFrame->flags & AV_FRAME_FLAG_KEY).

Comment 7

[Xi Ruoyao]

A kludge may be just passing AV_NOPTS_VALUE in place of pkt_dts, but I'm not sure if it will work at all.

Comment 8

[Artem S. Tashkinov]

Is it too much to ask to change the title to

"FFMpeg 8.0 support in Firefox" ?

Comment 9

[ChaseKnowlden]

Attachment: Bug 1962139 - Support ffmpeg 8.0 r?padenot

Comment 10

[grover92000]

The proposed change will not work for a couple of reasons. First, FFmpegRuntimeLinker.cpp needs to be updated so libavcodec.so.62 is in the list of supported libraries. FFmpegLibWrapper.cpp will probably also need to be updated with updates for AV_FUNC_62 and AV_FUNC_AVUTIL_62. Even after that streaming media will not play so I think some other change in ffmpeg breaks it.

Comment 11

[ChaseKnowlden]

Attachment: Bug 1962139 - Run clang-format r?padenot

Comment 12

[ChaseKnowlden]

Attachment: Bug 1962139 - Add libavcodec.so.62 to FFmpegRuntimeLinker.cpp r?padenot

Comment 13

[ChaseKnowlden]

YouTube Live still works fine for me.

Comment 14

[grover92000]

Doesn't work here with only ffmpeg-8 libraries installed. After installing the ffmpeg-7 libraries and running an strace on the startup of firefox shows it does open libavcodec.so.62 but must fail a check in the code because then it opens libavcodec.so.61 and runs fine with that.

Comment 15

[grover92000]

I got this working and you're close.
In FFmpegRuntimeLinker.cpp in addition to adding the .62 library names you need to add a case 62 in a few places. Look for case 61 and you'll see what I'm referring to.
In FFmpegLibWrapper.cpp you need AV_FUNC_62 and AV_FUNC_AVUTIL_62. See all places in that file that use 61 and make the same change for 62. In the same file change AV_FUNC(avcodec_close, AV_FUNC_AVCODEC_ALL). Replace AV_FUNC_AVCODEC_ALL with AV_FUNC_53 thru AV_FUNC_61. avcodec_close was removed in ffmpeg8.

Comment 16

[grover92000]

Attachment: firefox-144.0-ffmpeg8.patch

Attached is a patch that takes into account the changes by knowlden887 but actually fixes it to work with ffmpeg8.

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9519829 [details] [diff] [review]
firefox-144.0-ffmpeg8.patch

Removing release uplift request, release approval is only applicable to uplifts.
https://firefox-source-docs.mozilla.org/setup/contributing_code.html

Comment 18

[Kirill A. Korinsky]

grover92000, I think this hunk for FFmpegRuntimeLinker.cpp is wrong. I assume that you don't need to nuke 61

@@ -245,8 +253,8 @@ already_AddRefed<PlatformEncoderModule>
case 60:
module = FFmpegEncoderModule<60>::Create(&sLibAV);
break;

• case 61:

•  module = FFmpegEncoderModule<61>::Create(&sLibAV);
  

• case 62:

•  module = FFmpegEncoderModule<62>::Create(&sLibAV);
   break;
  

  default:
  module = nullptr;

Comment 20

[grover92000]

(In reply to Kirill A. Korinsky from comment #18)

grover92000, I think this hunk for FFmpegRuntimeLinker.cpp is wrong. I assume that you don't need to nuke 61

Good catch. You are correct. I'll attach an updated patch for 145.0b9.

Comment 21

[grover92000]

Attachment: firefox-145.0b9-ffmpeg8.patch

Comment 22

[Landry Breuil (:gaston)]

those patches are interesting only for downstream packagers that would want to fix things for their users, but imo the priority should shift to proper upstreaming.

chase, are you willing to update/revive the diffs that were pushed to phabricator ?

:padenot, what would be the best way forward to get this reviewed in the upcoming 147 cycle, with backport to 146 ?

Comment 23

[Landry Breuil (:gaston)]

[Tracking Requested - why for this release]:

i've tested 140.4.0esr, and it also doesnt play videos with ffmpeg8, so once commited it'll also need merge/backport to 140esr. I know it's too late for 145.0, maybe the next dot release..

Comment 24

[Landry Breuil (:gaston)]

Comment on attachment 9524599 [details] [diff] [review]
firefox-145.0b9-ffmpeg8.patch

doesnt apply as-is on 140esr:

Patching file dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp using Plan A...
Hunk #1 succeeded at 1205 (offset -101 lines).
Hunk #2 failed at 1232.
Hunk #3 failed at 1260.
Hunk #4 succeeded at 1698 (offset -21 lines).
Hunk #5 failed at 1743.
Hunk #6 failed at 1770.
Hunk #7 succeeded at 1680 with fuzz 1 (offset -119 lines).
Hunk #8 failed at 1763.
Hunk #9 failed at 1819.
No such line 2218 in input file, ignoring
Hunk #10 failed at 2281.
7 out of 10 hunks failed--saving rejects to dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp.rej

will need hand-massaging for the backport to 140esr

Comment 25

[Landry Breuil (:gaston)]

afaict (but i might be wrong), looking closely at the various diffs posted, at least in https://phabricator.services.mozilla.com/D264889, the ffmpeg62 dir is just a straight copy of ffmpeg61, which to my understanding isn't the right way to go.

https://bugzilla.mozilla.org/attachment.cgi?id=9524599 looks like a proper vendoring of the ffmpeg8 headers, but with (clang ? some?) formatting applied ? (checked by diffing the ffmpeg62 dir with an installed copy of ffmpeg 8.0) - and adding of the mozilla licence header to please some mozilla tool ?

+/* This Source Code Form is subject to the terms of the Mozilla Public                
+ * License, v. 2.0. If a copy of the MPL was not distributed with this                
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */     

in bug #1889978, the ffmpeg headers were added to clang-format-ignore, prettierignore and tools/rewriting/ThirdPartyPaths.txt (cf https://phabricator.services.mozilla.com/D206922#7160780 and https://hg-edge.mozilla.org/integration/autoland/rev/5970587d9a7a), should the same be done ?

Comment 26

[Paul Adenot (:padenot)]

(In reply to Landry Breuil (:gaston) from comment #22)

those patches are interesting only for downstream packagers that would want to fix things for their users, but imo the priority should shift to proper upstreaming.

chase, are you willing to update/revive the diffs that were pushed to phabricator ?

:padenot, what would be the best way forward to get this reviewed in the upcoming 147 cycle, with backport to 146 ?

Sorry I was on vacation and I just got back yesterday. I'll find someone to get this sorted this week, and if that doesn't happen I'll be able to work on this next week.

Comment 27

[Landry Breuil (:gaston)]

Attachment: bug-1962139-ffmpeg8-140esr.patch

the fuzz that made that the previous attachment (https://bugzilla.mozilla.org/attachment.cgi?id=9524599) didnt apply on 140.5esr was due to changes in bug #1934009 - this attached patch has been massaged (eg FFmpegLibWrapper.cpp & FFmpegVideoDecoder.cpp) so that it applied fine. Will runtime-test 140.5esr with it shortly.

Comment 28

[Landry Breuil (:gaston)]

Comment on attachment 9525881 [details] [diff] [review]
bug-1962139-ffmpeg8-140esr.patch

can confirm that this patch allows me to view videos in 140.5esr w/ ffmpeg8 libraries at runtime

Comment 29

[Landry Breuil (:gaston)]

padenot, my question was more about the separation of commits.. after looking at the patch stack in bug #1889978 , i also see that media/ffvpx has its own copy of libavcodec, which was updated separately in https://phabricator.services.mozilla.com/D206923.

in our case though, it's more 'to support running against a systemwide ffmpeg8', so i think this step can be left aside.

so if i get it right, to do the update properly reviewable we need separate commits:

• one vendoring the unmodified headers from ffmpeg8 (only the ones required, not 'all of the upstream ones' ?) and adding the moz.build glue (and making sure the various toolings dont complain about the codestyle/licence ?)

• one modifying the codepaths using them, sprinkling ifdefs here and there, but with which codestyle ? eg

+#  if LIBAVCODEC_VERSION_MAJOR < 62
   RefPtr<VideoData> vp = VideoData::CreateFromImage(
       mInfo.mDisplay, aOffset, TimeUnit::FromMicroseconds(aPts),
       TimeUnit::FromMicroseconds(aDuration), surface->GetAsImage(),
       !!mFrame->key_frame, TimeUnit::FromMicroseconds(mFrame->pkt_dts));
+#  else
+  RefPtr<VideoData> vp = VideoData::CreateFromImage(
+      mInfo.mDisplay, aOffset, TimeUnit::FromMicroseconds(aPts),
+      TimeUnit::FromMicroseconds(aDuration), surface->GetAsImage(),
+      !!(mFrame->flags & AV_FRAME_FLAG_KEY), TimeUnit::FromMicroseconds(mFrame->pkt_dts));
+#  endif

or rather set some local var like

+#  if LIBAVCODEC_VERSION_MAJOR < 62
+bool isKeyFrame =  !!mFrame->key_frame;
+#  else
+bool isKeyFrame = !!(mFrame->flags & AV_FRAME_FLAG_KEY);
+#  endif 
   RefPtr<VideoData> vp = VideoData::CreateFromImage(
       mInfo.mDisplay, aOffset, TimeUnit::FromMicroseconds(aPts),
       TimeUnit::FromMicroseconds(aDuration), surface->GetAsImage(),
-       !!mFrame->key_frame, TimeUnit::FromMicroseconds(mFrame->pkt_dts));
+       isKeyFrame, TimeUnit::FromMicroseconds(mFrame->pkt_dts));

or create a dedicate function like you proposed in https://phabricator.services.mozilla.com/D264889#inline-1451666 ? from my understanding there's two cases, that keyframe status, and that int64 pos being either packet->pos or mFrame->pkt_pos

• one modifying the huge lookup table of which function is present in which version (eg in https://searchfox.org/firefox-main/source/dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp#163) - but in which case does one use AV_FUNC_XX vs AV_FUNC_AVCODEC_XX or AV_FUNC_AVUTIL_XX ?)
• one adding libavxxx.62 to the lookup table (eg https://phabricator.services.mozilla.com/D266103 but i dont understand the linter complaint)

and i guess it needs much testing on try ?

Comment 30

[Landry Breuil (:gaston)]

another question on https://searchfox.org/firefox-main/source/dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp#248, for av_buffer_create, was the addition of AV_FUNC_61 an oversight ? shouldnt that be AV_FUNC_AVUTIL_61 for consistency ? afaict the function is still in libavutil/buffer.h for all the versions ?

Comment 31

[Landry Breuil (:gaston)]

       if (mUsingV4L2) {
+#      if LIBAVCODEC_VERSION_MAJOR < 62
         rv = CreateImageV4L2(mFrame->pkt_pos, GetFramePts(mFrame),
                              Duration(mFrame), aResults);
+#      else
+        rv = CreateImageV4L2(packet->pos, GetFramePts(mFrame), Duration(mFrame),
+                             aResults);
+#      endif
       } else {
+#      if LIBAVCODEC_VERSION_MAJOR < 62
         rv = CreateImageVAAPI(mFrame->pkt_pos, GetFramePts(mFrame),
                               Duration(mFrame), aResults);
+#      else
+        rv = CreateImageVAAPI(packet->pos, GetFramePts(mFrame),
+                              Duration(mFrame), aResults);
+#      endif
       }

this feels super ugly, and would probably benefit from less #if-nesting.

Comment 32

[Landry Breuil (:gaston)]

i've tried the 'add wrapper function route', with this:

--- i/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
+++ w/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
@@ -1062,6 +1062,22 @@ static int64_t GetFramePts(const AVFrame* aFrame) {
 #endif
 }
 
+static int64_t GetFramePos(const AVFrame* aFrame, const AVPacket* aPacket) {
+#if LIBAVCODEC_VERSION_MAJOR > 61
+  return aPacket->pos;
+#else
+  return aFrame->pkt_pos;
+#endif
+}
+
+static bool IsKeyFrame(const AVFrame* aFrame) {
+#if LIBAVCODEC_VERSION_MAJOR > 61
+  return !!(aFrame->flags & AV_FRAME_FLAG_KEY);
+#else
+  return !!aFrame->key_frame;
+#endif
+}
+
 #if LIBAVCODEC_VERSION_MAJOR >= 58
 void FFmpegVideoDecoder<LIBAV_VER>::DecodeStats::DecodeStart() {
   mDecodeStart = TimeStamp::Now();
@@ -1305,10 +1321,10 @@ MediaResult FFmpegVideoDecoder<LIBAV_VER>::DoDecode(
             RESULT_DETAIL("HW decoding is slow, switching back to SW decode"));
       }
       if (mUsingV4L2) {
-        rv = CreateImageV4L2(mFrame->pkt_pos, GetFramePts(mFrame),
+        rv = CreateImageV4L2(GetFramePos(mFrame, packet), GetFramePts(mFrame),
                              Duration(mFrame), aResults);
       } else {
-        rv = CreateImageVAAPI(mFrame->pkt_pos, GetFramePts(mFrame),
+        rv = CreateImageVAAPI(GetFramePos(mFrame, packet), GetFramePts(mFrame),
                               Duration(mFrame), aResults);
       }

but so far the build fails in ffmpeg/libavXX, which makes no real sense to me, since it built with the previous ifdef spaghetti..

 0:19.34 In file included from Unified_cpp_ffmpeg_libav530.cpp:56:
 0:19.34 /home/landry/src/m-c/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1069:18: error: no member named 'pkt_pos' in 'AVFrame'; did you mean 'pkt_pts'?
 0:19.34  1069 |   return aFrame->pkt_pos;                                                                
 0:19.34       |                  ^~~~~~~ 
 0:19.34       |                  pkt_pts                                                                 
 0:19.34 /home/landry/src/m-c/dom/media/platforms/ffmpeg/libav53/include/libavcodec/avcodec.h:1237:13: note: 'pkt_pts' declared here
 0:19.34  1237 |     int64_t pkt_pts;           
 0:19.34       |             ^                  
 0:19.37 In file included from Unified_cpp_ffmpeg_libav550.cpp:56:                                        
 0:19.37 /home/landry/src/m-c/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1069:18: error: no member named 'pkt_pos' in 'AVFrame'; did you mean 'pkt_pts'?
 0:19.37  1069 |   return aFrame->pkt_pos;           
 0:19.37       |                  ^~~~~~~                                                                 
 0:19.37       |                  pkt_pts                                                                 
 0:19.37 /home/landry/src/m-c/dom/media/platforms/ffmpeg/libav55/include/libavutil/frame.h:188:13: note: 'pkt_pts' declared here
 0:19.37   188 |     int64_t pkt_pts;                                                                     
 0:19.37       |             ^               
 0:19.37 In file included from Unified_cpp_ffmpeg_libav540.cpp:56:
 0:19.37 /home/landry/src/m-c/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:1069:18: error: no member named 'pkt_pos' in 'AVFrame'; did you mean 'pkt_pts'?                                                      
 0:19.37  1069 |   return aFrame->pkt_pos;                                                                
 0:19.37       |                  ^~~~~~~                                                                 
 0:19.37       |                  pkt_pts                                                                 
 0:19.37 /home/landry/src/m-c/dom/media/platforms/ffmpeg/libav54/include/libavcodec/avcodec.h:1095:13: note: 'pkt_pts' declared here
 0:19.37  1095 |     int64_t pkt_pts;         

Comment 33

[Landry Breuil (:gaston)]

strangely, using a local variable instead seems to work for that frame/packet position/offset thing:

@@ -1270,6 +1288,12 @@ MediaResult FFmpegVideoDecoder<LIBAV_VER>::DoDecode(
 #  endif
 
     int res = mLib->avcodec_receive_frame(mCodecContext, mFrame);
+    int64_t fpos =
+#  if LIBAVCODEC_VERSION_MAJOR > 61
+        packet->pos;
+#  else
+        mFrame->pkt_pos;
+#  endif
     if (res == int(AVERROR_EOF)) {
       if (MaybeQueueDrain(aResults)) {
         FFMPEG_LOG("  Output buffer shortage.");
@@ -1305,11 +1329,11 @@ MediaResult FFmpegVideoDecoder<LIBAV_VER>::DoDecode(
             RESULT_DETAIL("HW decoding is slow, switching back to SW decode"));
       }
       if (mUsingV4L2) {
-        rv = CreateImageV4L2(mFrame->pkt_pos, GetFramePts(mFrame),
-                             Duration(mFrame), aResults);
+        rv = CreateImageV4L2(fpos, GetFramePts(mFrame), Duration(mFrame),
+                             aResults);
       } else {
-        rv = CreateImageVAAPI(mFrame->pkt_pos, GetFramePts(mFrame),
-                              Duration(mFrame), aResults);
+        rv = CreateImageVAAPI(fpos, GetFramePts(mFrame), Duration(mFrame),
+                              aResults);
       }

(at least my build goes further)

Comment 34

[Landry Breuil (:gaston)]

(In reply to Landry Breuil (:gaston) from comment #25)

afaict (but i might be wrong), looking closely at the various diffs posted, at least in https://phabricator.services.mozilla.com/D264889, the ffmpeg62 dir is just a straight copy of ffmpeg61, which to my understanding isn't the right way to go.

i retract that comment, having relooked properly, the ffmpeg62 headers in https://phabricator.services.mozilla.com/D264889 are an exact copy of the ones from ffmpeg8;

~/src/m-c/dom/media/platforms/ffmpeg/ $moz-phab patch -n bug-1962139-chasek D264889                                                       
~/src/m-c/dom/media/platforms/ffmpeg/ $git checkout 2570a98f68f9b32ec6e88fb78faba7f5498aa900
~/src/m-c/dom/media/platforms/ffmpeg/ $for f in ffmpeg62/include/libavcodec/* ; do diff -u $f /usr/local/$(echo $f |cut -d / -f2-) ; done 
~/src/m-c/dom/media/platforms/ffmpeg/ $for f in ffmpeg62/include/libavutil/* ; do diff -u $f /usr/local/$(echo $f |cut -d / -f2-) ; done  

https://phabricator.services.mozilla.com/D266102 misled me by adding licence headers and clang-format reformatting... and removing all the headers that had been (mistakenly?) added in https://phabricator.services.mozilla.com/D264889 but were not used.

Comment 35

[Landry Breuil (:gaston)]

Attachment: Bug 1962139 - Vendor ffmpeg 8.0 headers r?#media-playback-reviewers

taken straight from ffmpeg, without any kind of formatting/licence changes

Comment 36

[Landry Breuil (:gaston)]

Attachment: Bug 1962139 - Adapt the macros/defines for function detection in ffmpeg8 r?#media-playback-reviewers

avcodec_close() is the only function that was removed from the API

Comment 37

[Landry Breuil (:gaston)]

Attachment: Bug 1962139 - Adapt FFmpegVideoDecoder for ffmpeg8 r?#media-playback-reviewers

Provide IsKeyFrame() helper handling various versions, and use it where
appropriate. Also, In ffmpeg 8 the offset is to be found in the packet struct,
not anymore in the frame struct, so use a variable local to DoDecode()

Comment 38

[Landry Breuil (:gaston)]

Attachment: Bug 1962139 - FF_PROFILE defines were renamed to AV_PROFILE in ffmpeg8 r?#media-playback-reviewers

Comment 39

[Landry Breuil (:gaston)]

Attachment: Bug 1962139 - Enable ffmpeg8 support in FFmpegRuntimeLinker.cpp r?#media-playback-reviewers

Comment 40

[Landry Breuil (:gaston)]

I've pushed what i have here, rebased on top of m-c, with more atomic commits. Builds fine on OpenBSD, will definitely need build tests on other platforms, and runtime-testing, and i guess try runs i suppose i don't have access to anymore.

Comment 41

[Landry Breuil (:gaston)]

:padenot, since most of the stack is reviewed now, what is the next step ? can you push a try run with the stack, even if i doubt that'd exercise the new codepaths, at least it'll build-test them ? or land everything and wait for fireworks ?

who can do some actual runtime testing ? i know patches posted here (or variations of them) are in use in downstream distros/oses... but i dunno if that counts.

i'd really like to get this merged so that it can get backported to 146, if timing allows it..

Comment 42

[Landry Breuil (:gaston)]

• alpine linux pins to ffmpeg7: https://gitlab.alpinelinux.org/alpine/aports/-/commit/2993d5a900bce419f2c3559cfd5595f5ae1eedbd
• gentoo pins to <8: https://bugs.gentoo.org/962454
• archlinux to 4.4: https://gitlab.archlinux.org/archlinux/packaging/packages/firefox/-/commit/124858160b0698e6f50519c258ec3c004e7fb8e2
• debian experimental has ffmpeg 8 but i dunno how it's handled yet in the firefox package ? :glandium ?
• freebsd doesnt have ffmpeg8 but they're working on it
• pkgsrc depends on 7 per https://github.com/NetBSD/pkgsrc/blob/trunk/www/firefox/mozilla-common.mk#L281
• openbsd has the patch from this bug: https://github.com/openbsd/ports/commit/1d731220440c01ddd6ce6ef88b223ed9d78eeb40
• nixpkgs pins to 7: https://github.com/NixOS/nixpkgs/commit/6268044a41b4bdbd6ec95db73f8336cb41c9e21b

Comment 43

[Paul Adenot (:padenot)]

Ok I'm back, sorry for the wait:

A couple try pushes, respectively trunk and beta:

https://treeherder.mozilla.org/jobs?repo=try&revision=f8ba638316764f050c380ff0b6876210d76a1e0b
https://treeherder.mozilla.org/jobs?repo=try&revision=0ad3af88782376e86c647fc24886c83c45bf040a

if this is green, I'll click the button to land on trunk, and I'll ask for beta uplift. Thanks a lot for working on this, we should have done it earlier.

Comment 44

[Landry Breuil (:gaston)]

thanks a lot ! besides one unrelated js linting issue, they look pretty green to me :)

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7e23c7c18013
https://hg.mozilla.org/integration/autoland/rev/19e0df8033b2
Vendor ffmpeg 8.0 headers r=media-playback-reviewers,frontend-codestyle-reviewers,mossop,alwu
https://github.com/mozilla-firefox/firefox/commit/704e01bf9fe1
https://hg.mozilla.org/integration/autoland/rev/f23d592e80b9
Adapt the macros/defines for function detection in ffmpeg8 r=media-playback-reviewers,alwu
https://github.com/mozilla-firefox/firefox/commit/ac2c1ca4323d
https://hg.mozilla.org/integration/autoland/rev/568e210639f7
Adapt FFmpegVideoDecoder for ffmpeg8 r=media-playback-reviewers,alwu
https://github.com/mozilla-firefox/firefox/commit/7d33341f693e
https://hg.mozilla.org/integration/autoland/rev/eb8352183323
FF_PROFILE defines were renamed to AV_PROFILE in ffmpeg8 r=media-playback-reviewers,alwu
https://github.com/mozilla-firefox/firefox/commit/6aff720ee5ac
https://hg.mozilla.org/integration/autoland/rev/b1660dee0419
Enable ffmpeg8 support in FFmpegRuntimeLinker.cpp r=media-playback-reviewers,padenot

Comment 46

[Donal Meehan [:dmeehan]]

ESR140 is only receiving stability fixes

Comment 47

[Paul Adenot (:padenot)]

Attachment: Bug 1962139 - Vendor ffmpeg 8.0 headers

taken straight from ffmpeg, without any kind of formatting/licence changes

Original Revision: https://phabricator.services.mozilla.com/D272252

Comment 48

[Paul Adenot (:padenot)]

Attachment: Bug 1962139 - Adapt the macros/defines for function detection in ffmpeg8

avcodec_close() is the only function that was removed from the API

Original Revision: https://phabricator.services.mozilla.com/D272253

Comment 49

[Paul Adenot (:padenot)]

Attachment: Bug 1962139 - Adapt FFmpegVideoDecoder for ffmpeg8

Provide IsKeyFrame() helper handling various versions, and use it where
appropriate. Also, In ffmpeg 8 the offset is to be found in the packet struct,
not anymore in the frame struct, so use a variable local to DoDecode()

Original Revision: https://phabricator.services.mozilla.com/D272254

Comment 50

[Paul Adenot (:padenot)]

Attachment: Bug 1962139 - FF_PROFILE defines were renamed to AV_PROFILE in ffmpeg8

Original Revision: https://phabricator.services.mozilla.com/D272255

Comment 51

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Increasingly high number of video playback breakage on Linux Desktop caused by a version mismatch between what Firefox supports and what the system has, Firefox was missing support for the latest version of ffmpeg libraries.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Patches tested by various downstream packagers.
• String changes made/needed: none
• Is Android affected?: no

Comment 52

[Paul Adenot (:padenot)]

Attachment: Bug 1962139 - Enable ffmpeg8 support in FFmpegRuntimeLinker.cpp

Original Revision: https://phabricator.services.mozilla.com/D272256

Comment 53

[Paul Adenot (:padenot)]

Gaston, as you've seen above, I've queued this for landing and requested Beta uplift.

I should be alright since it was tested a lot already by downstream, but anybody can ping me directly (here or [e-mail]) for anything -- I should be able to answer with a low latency now.

Comment 54

[Landry Breuil (:gaston)]

(In reply to Donal Meehan [:dmeehan] from comment #46)

ESR140 is only receiving stability fixes

well, that's ... a sad decision. I guess it sort of makes sense since ESR might be used on/targetting 'stable' branches of distros/OSes who dont update ffmpeg, but that doesnt cover all cases. If that decision is reevaluated, i have a patch ready for esr140 branch around.

Thanks paul for landing it (and the backport request)

Comment 55

[Joonas Niilola]

(In reply to Landry Breuil (:gaston) from comment #54)

If that decision is reevaluated, i have a patch ready for esr140 branch around.

Such patch would still be welcome here for any downstream distributors to apply on their judgement :) ESR codebase should be stable enough for it to carry through 140esr series.

Comment 56

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/19e0df8033b2
https://hg.mozilla.org/mozilla-central/rev/f23d592e80b9
https://hg.mozilla.org/mozilla-central/rev/568e210639f7
https://hg.mozilla.org/mozilla-central/rev/eb8352183323
https://hg.mozilla.org/mozilla-central/rev/b1660dee0419

Comment 57

[Landry Breuil (:gaston)]

(In reply to Joonas Niilola from comment #55)

(In reply to Landry Breuil (:gaston) from comment #54)

If that decision is reevaluated, i have a patch ready for esr140 branch around.

Such patch would still be welcome here for any downstream distributors to apply on their judgement :) ESR codebase should be stable enough for it to carry through 140esr series.

if you look, there's https://bugzilla.mozilla.org/show_bug.cgi?id=1962139#c27 that we (OpenBSD) use in our ports-tree. In that context, i was mentioning a patch via phabricator, for proper upstreaming. all other commits should apply as-is.

Comment 58

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/501b9947b2bc
https://hg.mozilla.org/releases/mozilla-beta/rev/95bf2b3b01f0
https://github.com/mozilla-firefox/firefox/commit/865da429a675
https://hg.mozilla.org/releases/mozilla-beta/rev/174b44d28a45
https://github.com/mozilla-firefox/firefox/commit/9b678233fed5
https://hg.mozilla.org/releases/mozilla-beta/rev/d8831eb77682
https://github.com/mozilla-firefox/firefox/commit/9db05284a73d
https://hg.mozilla.org/releases/mozilla-beta/rev/0cd315c8e938
https://github.com/mozilla-firefox/firefox/commit/56319d56bcb0
https://hg.mozilla.org/releases/mozilla-beta/rev/78a37678cbd4