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

[FFmpeg] chromium patch #38683

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Neumann-A
Copy link
Contributor

Required to build qtwebengine and probably chromium itself with ffmpeg on linux-dynamic

#endif

+// Chromium: We use the internal field first_dts vvv
+int64_t av_stream_get_first_dts(const AVStream *st)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this fix been synced upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No and it wont be. FFMPEG removed previously available API without giving a migration guide, so chromium struggles to remove the usage of this symbol. Please read the discussion in https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://issues.chromium.org/issues/40218408&ved=2ahUKEwjAiNSG5oSGAxVmc_EDHQCIC1IQFnoECA4QAQ&usg=AOvVaw0ZS9B5G7XW78802cwojWfv

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this patch from? Has it been accepted in chromium or elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original the patch was from https://aur.archlinux.org/cgit/aur.git/tree/040-ffmpeg-add-av_stream_get_first_dts-for-chromium.patch?h=ffmpeg-intel-full-git but I had to adjust it to the ffmpeg version vcpkg has.

After a bit of googling you can also find it here https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/95aab0fd83619408995720ce53d7a74790580220%5E%21/
However this also seems to be for a different ffmpeg version

I mean you can scan the chromium repo:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/ffmpeg/libavformat/avformat.h;l=1174

https://source.chromium.org/chromium/chromium/src/+/main:third_party/ffmpeg/libavformat/utils.c;l=61?q=av_stream_get_first_dts&sq=&ss=chromium%2Fchromium%2Fsrc

I moved the patch into mux_utils.c since the patch did not apply cleanly and I was looking for where av_stream_get_end_pts from the original patch went. At least it made sense for me to move it there. I can adjust the patch to move it back into utils.c but I don't think it makes any difference for the final library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also av_stream_get_end_pts compared to av_stream_get_first_dts felt kind of related by name.

@jimwang118 jimwang118 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 11, 2024
@jimwang118
Copy link
Contributor

Compile test pass with following triplets:

x64-linux-dynamic

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label May 11, 2024
@vicroms vicroms marked this pull request as draft May 23, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines requires:discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants