jellyfin / jellyfin-ffmpeg

FFmpeg for Jellyfin
https://jellyfin.org
Other
436 stars 118 forks source link

fix VT tone mapping and add transpose_vt #352

Closed gnattu closed 4 months ago

gnattu commented 4 months ago

Changes

This PR:

Issues

gnattu commented 4 months ago

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

nyanmisaka commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago
    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 commented 4 months ago

Format check relaxed.