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

New upstream version 7.0.2 #424

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Conversation

nyanmisaka
Copy link
Member

@nyanmisaka nyanmisaka commented Aug 1, 2024

Vulkan patches are not yet ready.

Changes

  • New upstream version 7.0.2
  • Rename package to jellyin-ffmpeg7
  • Update build scripts and dependencies

Issues

uspp_filter_deps="gpl avcodec"
Index: FFmpeg/libavfilter/vf_transpose_vt.c
===================================================================
--- FFmpeg.orig/libavfilter/vf_transpose_vt.c
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to make a version merges two of them. But this is good enough for now.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

With 7.0.1+backported patches ffmpeg now correctly set the buffer attachments for videotoolbox and now the tonemap pipeline needs to be slightly modified because the old one works with some assumptions with wrong colors, and the HDR attachments is not properly removed doing tone-mapping to 10bit now.

@nyanmisaka
Copy link
Member Author

nyanmisaka commented Aug 1, 2024

and the HDR attachments is not properly removed doing tone-mapping to 10bit now.

Is CVBufferRemoveAttachment() needed here?

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

and the HDR attachments is not properly removed doing tone-mapping to 10bit now.

Is CVBufferRemoveAttachment() needed here?

We need to override the kCVImageBufferTransferFunctionKey I think, but I will look at this later. Once this one is merged I will submit a PR fix it.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

Well interesting, scale_vt is still producing wrong color without manually hwupload. I guess the upstream missed up something once again.

@nyanmisaka
Copy link
Member Author

Well interesting, scale_vt is still producing wrong color without manually hwupload. I guess the upstream missed up something once again.

These two fixes have been included in the backport patches.
FFmpeg/FFmpeg@1fa7554
FFmpeg/FFmpeg@cd9ceae

Is this possible related to the newly added SDK version check?
FFmpeg/FFmpeg@85706f5
FFmpeg/FFmpeg@2fc37c4
FFmpeg/FFmpeg@ca7fcf5

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

Well interesting, scale_vt is still producing wrong color without manually hwupload. I guess the upstream missed up something once again.

These two fixes have been included in the backport patches. FFmpeg/FFmpeg@1fa7554 FFmpeg/FFmpeg@cd9ceae

Is this possible related to the newly added SDK version check? FFmpeg/FFmpeg@85706f5 FFmpeg/FFmpeg@2fc37c4 FFmpeg/FFmpeg@ca7fcf5

The CGColorSpace generated from its provided buffer is wrong and creates an SDR colorspace.

@nyanmisaka
Copy link
Member Author

The CGColorSpace generated from its provided buffer is wrong and creates an SDR colorspace.

Is this also reproducible in the ffmpeg master branch? I saw your discussion on the mailing list and thought it was fixed.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

The CGColorSpace generated from its provided buffer is wrong and creates an SDR colorspace.

Is this also reproducible in the ffmpeg master branch? I saw your discussion on the mailing list and thought it was fixed.

I identified the problem but I found that it is really funny. The whole sdk version checking drama is to get the attachments from the pixel buffer, but the attachments get from that buffer will always creates a non-HDR buffer. The very first implementation allocated another dedicated dictionary which only specifies the color primaries, the transfer function and the color matrix, which will properly create an HDR colorspace and make scale_vt happy. I need to submit this to the upstream I think.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

The good news is that, a framebuffer with properly set attachments brings slightly performance improvements over hwupload, instead of regression in earlier version. So maybe we can get rid of hwupload in jellyfin 10.10.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

Patch submitted to upstream: https://ffmpeg.org//pipermail/ffmpeg-devel/2024-August/331960.html

@nyanmisaka
Copy link
Member Author

Patch submitted to upstream: https://ffmpeg.org//pipermail/ffmpeg-devel/2024-August/331960.html

defined but not used?

static CFDictionaryRef vt_cv_buffer_copy_attachments(CVBufferRef buffer,
                                                     CVAttachmentMode attachment_mode)

redundant braces?

#if (TARGET_OS_OSX && __MAC_OS_X_VERSION_MAX_ALLOWED >= 100800) || \
    (TARGET_OS_IOS && __IPHONE_OS_VERSION_MAX_ALLOWED >= 100000)
    if (__builtin_available(macOS 10.8, iOS 10, *)) {
        colorspace = CVImageBufferCreateColorSpaceFromAttachments(attachments);
    }
#endif

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

Patch submitted to upstream: https://ffmpeg.org//pipermail/ffmpeg-devel/2024-August/331960.html

defined but not used?

static CFDictionaryRef vt_cv_buffer_copy_attachments(CVBufferRef buffer,
                                                     CVAttachmentMode attachment_mode)

This can be used because the real cause is simpler than I originally thought.

redundant braces?

#if (TARGET_OS_OSX && __MAC_OS_X_VERSION_MAX_ALLOWED >= 100800) || \
    (TARGET_OS_IOS && __IPHONE_OS_VERSION_MAX_ALLOWED >= 100000)
    if (__builtin_available(macOS 10.8, iOS 10, *)) {
        colorspace = CVImageBufferCreateColorSpaceFromAttachments(attachments);
    }
#endif

CVImageBufferCreateColorSpaceFromAttachments is only available in macOS 10.8 so this is still necessary.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

See: https://github.com/FFmpeg/FFmpeg/blob/1b2a925e94c772c59a88c03c1654bddf6aff0ca2/libavutil/hwcontext_videotoolbox.c#L600

The color primaries value is wrongly assigned to color transfer key and that's why the attachments copied from the pixel buffer will always create an SDR colorspace. After correcting this the original code works properly.

@nyanmisaka
Copy link
Member Author

See: https://github.com/FFmpeg/FFmpeg/blob/1b2a925e94c772c59a88c03c1654bddf6aff0ca2/libavutil/hwcontext_videotoolbox.c#L600

The color primaries value is wrongly assigned to color transfer key and that's why the attachments copied from the pixel buffer will always create an SDR colorspace. After correcting this the original code works properly.

colorxxx is hard to distinguish at a glance... It's necessary to use underscores or camelCase.

@gnattu
Copy link
Member

gnattu commented Aug 1, 2024

@gnattu
Copy link
Member

gnattu commented Aug 2, 2024

This version currently does not contain the nop vt_device_derive because it is not needed for upstream. But our current server code uses hwupload=derive_device=videotoolbox a lot. Should we add that as well to support current server?

@nyanmisaka
Copy link
Member Author

This version currently does not contain the nop vt_device_derive because it is not needed for upstream. But our current server code uses hwupload=derive_device=videotoolbox a lot. Should we add that as well to support current server?

I remembered it. Will re-add it as a separate patch since upstream will never accept it.

Signed-off-by: nyanmisaka <nst799610810@gmail.com>
Signed-off-by: nyanmisaka <nst799610810@gmail.com>
@nyanmisaka nyanmisaka changed the title [WIP] New upstream version 7.0.1 [WIP] New upstream version 7.0.2 Aug 3, 2024
Signed-off-by: nyanmisaka <nst799610810@gmail.com>
Signed-off-by: nyanmisaka <nst799610810@gmail.com>
Signed-off-by: nyanmisaka <nst799610810@gmail.com>
@nyanmisaka nyanmisaka changed the base branch from jellyfin to jellyfin-7.0 August 7, 2024 12:10
@nyanmisaka nyanmisaka marked this pull request as ready for review August 7, 2024 12:13
@nyanmisaka nyanmisaka changed the title [WIP] New upstream version 7.0.2 New upstream version 7.0.2 Aug 7, 2024
@nyanmisaka nyanmisaka merged commit c9dd753 into jellyfin:jellyfin-7.0 Aug 7, 2024
27 checks passed
@nyanmisaka
Copy link
Member Author

Merging to jellyfin-7.0 branch for now.

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.

Upgrade to FFmpeg 7.0
2 participants