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

fix VT tone mapping and add transpose_vt #352

Merged
merged 5 commits into from
Mar 9, 2024
Merged

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Mar 9, 2024

Changes

This PR:

  • Added a back-ported transpose_vt filter which uses CoreImage that works on macOS 12
  • Added format option for scale_vt filter. Currently only supports nv12 and p010

Issues

@gnattu gnattu requested a review from nyanmisaka March 9, 2024 03:36
@gnattu
Copy link
Member Author

gnattu commented Mar 9, 2024

A format option is added for scale_vt. I limit the valid option to nv12 and p010 for now.

@nyanmisaka
Copy link
Member

A format option is added for scale_vt. I limit the valid option to nv12 and p010 for now.

https://github.com/FFmpeg/FFmpeg/blob/64634e809f2e7b6c1a1ea3f6952a17c5915c3f22/libavcodec/videotoolbox.c#L1139-L1181

Can I assume that VtTransfer and VtDecoder share all input and output formats? If so I guess they could be added as well.

@gnattu
Copy link
Member Author

gnattu commented Mar 9, 2024

Can I assume that VtTransfer and VtDecoder share all input and output formats? If so I guess they could be added as well.

The VTPixelTransferSession may not support all conversions to/from the formats supported by VideoToolbox, and NV12 and P010 are confirmed to work as the "safest" options. The encoder would also produce problematic outputs when a supported but not ideal input is given, just like the BGRA case we were seeing. If we want to add pixel formats that we are not sure will work, we can simply skip such checks altogether and let VideoToolbox error out if necessary. However, this approach cannot cover the case where the encoder produces wrong outputs.

@nyanmisaka
Copy link
Member

nyanmisaka commented Mar 9, 2024

Can I assume that VtTransfer and VtDecoder share all input and output formats? If so I guess they could be added as well.

The VTPixelTransferSession may not support all conversions to/from the formats supported by VideoToolbox, and NV12 and P010 are confirmed to work as the "safest" options. The encoder would also produce problematic outputs when a supported but not ideal input is given, just like the BGRA case we were seeing. If we want to add pixel formats that we are not sure will work, we can simply skip such checks altogether and let VideoToolbox error out if necessary. However, this approach cannot cover the case where the encoder produces wrong outputs.

Is it possible to check nv12 and p010le only when the user sets a different output format? (making it slightly relaxed just like upstream) If it's just scaling and no format conversion involved, I think it should be safe. (To make it not a breaking change)

@gnattu
Copy link
Member Author

gnattu commented Mar 9, 2024

Is it possible to check nv12 and p010le only when the user sets a different output format?

I think it already behaves like this?

https://github.com/gnattu/jellyfin-ffmpeg/blob/ac2e8120c170b1668470d348e7a842049adea315/debian/patches/0061-backport-scale-vt.patch#L308

out_format = (s->format == AV_PIX_FMT_NONE) ? hw_frame_ctx_in->sw_format : s->format;

When the user sets no format, it would be AV_PIX_FMT_NONE, and in this case, it will just use whatever format is passed in.

@nyanmisaka
Copy link
Member

    out_format = (s->format == AV_PIX_FMT_NONE) ? hw_frame_ctx_in->sw_format : s->format;
    if (!format_is_supported(out_format)) {
        av_log(s, AV_LOG_ERROR, "Unsupported output format: %s\n",
               av_get_pix_fmt_name(out_format));
        return AVERROR(ENOSYS);
    }

//hw_frame_ctx_in->sw_format == p210le
//format_is_supported(hw_frame_ctx_in->sw_format) == 0

Currently, even if the user does not set the format option, an error will be returned when the input of scale_vt is non-nv12/p010le. There is no such restriction upstream.

@gnattu
Copy link
Member Author

gnattu commented Mar 9, 2024

Format check relaxed.

@gnattu gnattu merged commit f319aa0 into jellyfin:jellyfin Mar 9, 2024
31 checks passed
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