Close JPEG-XL decoding/screenshot support, #3852 #4367

low-batt · 2023-04-20T00:30:51Z

This commit will:

Remove ppm, pgm, pgmyuv and tga from the Preferences.ScreenshotFormat enum
Add webp and jxl to the Preferences.ScreenshotFormat enum
Add WebP (.webp) and JPEG XL (.jxl) as choices for the Format setting in the Screenshots section of General settings

NOTE that removing enumeration cases breaks backward compatibility as the new cases reuse the tag values previously assigned to ppm and pgm. These four screenshot formats were dropped by mpv and removed from IINA's setting panel back in 2017. Anyone using these old formats would have had to update their settings therefore it should be acceptable to reuse the tag values.

The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork.

I have read CONTRIBUTING.md
This implements/fixes issue JPEG-XL decoding/screenshot support? #3852.

Description:

low-batt · 2023-04-20T00:40:04Z

I marked this PR as a draft because taking a JPEG XL screenshot while playing a HDR video did did not generate a screenshot that matched the video.

Also there is a second issue in that with Show previews after taking screenshots enabled the OSD only shows Screenshot Captured. This is because NSImage is unable to create an image from a JPEG XL file as macOS does not support this kind of file. The code in PlayerCore.screenshotCallback has a guard statement to handle this. However not only does the OSD not display the screenshot, it does not provide the DELETE, EDIT and REVEAL buttons. The DELETE and REVEAL buttons are definitely still applicable. It is desirable to at least provide the REVEAL button.

To test JPEG XL I played a downloaded copy of the YouTube video The World in HDR in 4K (ULTRA HD) using IINA+ build 170 and took a screenshot. I then repeated this with IINA

I tested IINA with the changes in this PR applied to the changes in branch close-4358 which sets screenshot-tag-colorspace to true using mpv 0.35.1 and FFmpeg 6.

The screenshot generated by IINA+ uses this color space:

Color space: RGB, D65, Rec.2100 primaries, PQ transfer

When displayed in IINA or IINA+ that screenshot displays in HDR and looks like the video.

The screenshot generated by IINA uses this color space:

Color space: RGB, D65, sRGB primaries, sRGB transfer

When viewed in IINA or IINA+ that screenshot is not displayed in HDR and is washed out.

Full IINA+ Screenshot Info:

low-batt@gag Screenshots$ ls -la The\ World\ in\ HDR\ in\ 4K\ \(ULTRA\ HD\)-0001.jxl 
-rw-r--r--@ 1 low-batt  staff  456600 Apr 19 19:17 The World in HDR in 4K (ULTRA HD)-0001.jxl
low-batt@gag Screenshots$ jxlinfo -v The\ World\ in\ HDR\ in\ 4K\ \(ULTRA\ HD\)-0001.jxl 
JPEG XL image, 3840x2160, lossy, 16-bit RGB
num_color_channels: 3
num_extra_channels: 0
intensity_target: 10000.000000 nits
min_nits: 0.000000
relative_to_max_display: 0
linear_below: 0.000000
have_preview: 0
have_animation: 0
Intrinsic dimensions: 3840x2160
Orientation: 1 (Normal)
Color space: RGB, D65, Rec.2100 primaries, PQ transfer function, rendering intent: Relative
low-batt@gag Screenshots$

Full IINA Screenshot Info:

low-batt@gag Screenshots$ ls -la The\ World\ in\ HDR\ in\ 4K\ \(ULTRA\ HD\)-0002.jxl 
-rw-r--r--@ 1 low-batt  staff  415701 Apr 19 19:25 The World in HDR in 4K (ULTRA HD)-0002.jxl
low-batt@gag Screenshots$ jxlinfo -v The\ World\ in\ HDR\ in\ 4K\ \(ULTRA\ HD\)-0002.jxl 
JPEG XL image, 3840x2160, lossy, 16-bit RGB
num_color_channels: 3
num_extra_channels: 0
have_preview: 0
have_animation: 0
Intrinsic dimensions: 3840x2160
Orientation: 1 (Normal)
Color space: RGB, D65, sRGB primaries, sRGB transfer function, rendering intent: Relative
low-batt@gag Screenshots$

@CarterLi Any guess as to the cause of the difference between IINA and IINA+? IINA+ was built with mpv master whereas IINA is using the mpv 0.35.1 release, so a difference in mpv is one possibility.

low-batt · 2023-05-25T23:03:14Z

HELP!!

I'm requesting help on this one as I lack experience in this area.

@CarterLi, I'm hoping for some advice on the HDR aspects.

First, on the issue with mpv 0.35.1 and JPEG XL screenshots of HDR videos. I believe that is due to the mpv issue mpv-player/mpv#10988. The one line fix for that issue was merged right after the 0.35.1 release was cut. I will prepare a patch to mpv for that issue.

With a working mpv and the screenshot format set to JPEG XL you can take a screenshot of a HDR video that looks like the video when viewed using IINA. The screenshot preview looks like EDR is working, but I'm unsure if the presentation is fully correct.

What could be improved is the screenshot preview you see for a HDR video if HDR is turned off. It is clipped instead of tone mapped. Anyone know how to fix that?

I was unable to get FFmpeg to generate what I believe is the bitmap format required by Apple. I added a workaround for that. Is there another approach I should be using?

I included support for the display P3 color spaces, following what VideoView does, but I was unable to find a video that tests that.

Anyway, this PR could use an intense mean detailed critical review.

iina/FFmpegController.m

CarterLi · 2023-05-26T00:49:13Z

If I were you, I would use MPV (VideoView) to display the image, so that I could reuse most of the existing code, and would not have HDR issues.

low-batt · 2023-05-26T01:56:02Z

An interesting idea. I initially discounted using mpv due to issues with integrating with the OSD and the heavy overhead of dealing with a mpv core. I will give it more thought, however I believe it is going to be even harder to get that to work in the other use case which I did not mention. The other HDR issue we have not addressed is thumbnails for HDR videos. We could generate JPEG XL thumbnails that would show HDR thumbnails. The thumbnails all live in one file in a proprietary IINA format. Not sure how we could get mpv to read that. I suppose we could generate a video whose frames are the thumbnails. Something like that maybe. I will think on this.

low-batt · 2023-05-26T20:06:07Z

I played around with FFmpeg today from the command line and was able to generate from a 10 minute video at tiny video with only 65 frames, all of them keyframes. Something like that should allow us to use mpv for displaying HDR thumbnails.

One reason I was focused on creating a NSImage object due to this code from PlayerCore.screenshotCallback:

NSPasteboard.general.writeObjects([image])

If we use mpv to read the screenshot, we'd need a way to put the decoded screenshot into the pasteboard. I still need to figure that out.

The case for using mpv is that this is the best way to ensure that these images match the video. That is a really compelling argument.

To do this cleanly is going to require a lot of refactoring. It would be inappropriate to use PlayerCore objects for this. We would not want to use a VideoView with a CVDisplayLink for a still image. I'm currently thinking it would not be a good idea to add a lot of checks to classes to cover still images. Possibly we should introduce protocols, abstract base class with separate video and image implementations. Something like:

MPVController (protocol)
MPVControllerCommon
MPVControllerImage
MPVControllerVideo

I have a some other issues I need to turn my attention to at the moment, but when I have time I will hack up an ugly test that uses mpv to make sure there isn't some sort of blocker to this plan.

I'm thinking for now we continue to review this PR with the understanding that it is temporary code likely to be replaced later.

saagarjha · 2023-06-03T13:33:27Z

iina/FFmpegController.h

+/// - Parameter url: The URL identifying the image.
+/// - Returns: An initialized NSImage object or null if the method cannot create an image representation from the contents of the
+///       specified URL.
+ (nullable NSImage *)createNSImageWithContentsOf:(nonnull NSURL *)url;


Suggested change

+ (nullable NSImage *)createNSImageWithContentsOf:(nonnull NSURL *)url;

+ (nullable NSImage *)createNSImageWithContentsOfURL:(nonnull NSURL *)url;

Fixed. I was trying to pattern this after this NSImage initializer. But I could not get the Swift name translation to use contentsOf for the parameter name. I had to add "With".

saagarjha · 2023-06-03T13:34:12Z

iina/FFmpegController.m

+#import "IINA-Swift.h"
+
+#define ERROR(msg, ...) [FFmpegLogger error:([NSString stringWithFormat:(msg), ##__VA_ARGS__])];
+#define LOG(msg, ...) [FFmpegLogger debug:([NSString stringWithFormat:(msg), ##__VA_ARGS__])];


I would call this INFO or similar personally, "LOG" isn't really a log level

The problem was the correct name would be DEBUG and that name is taken. I added a LOG_ prefix to these to allow uniform naming.

saagarjha · 2023-06-03T13:40:50Z

iina/FFmpegController.m

+#if DEBUG
+    LOG(@"Creating image with contents of file: %s", cFilename)
+#endif


Consider moving this up so the strdup/free pair are closer together. I think technically the copy isn't required anyways

Good catch. I patterned this after getPeeksForFile figuring it must be correct, however using fileSystemRepresentation directly works fine.

saagarjha · 2023-06-03T13:45:20Z

iina/FFmpegController.m

+    }
+    return image;
+  }
+  @finally {


I thought this try-finally pattern was better than trying to cleanup in each of those error returns. If this was Swift I'd use defer after each allocation.

This commit will: - Remove ppm, pgm, pgmyuv and tga from the Preferences.ScreenshotFormat enum - Add webp and jxl to the Preferences.ScreenshotFormat enum - Add WebP (.webp) and JPEG XL (.jxl) as choices for the Forma` setting in the Screenshots section of General settings NOTE that removing enumeration cases breaks backward compatibility as the new cases reuse the tag values previously assigned to ppm and pgm. These four screenshot formats were dropped by mpv and removed from IINA's setting panel back in 2017. Anyone using these old formats would have had to update their settings therefore it should be acceptable to reuse the tag values. The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork. Co-authored-by: CarterLi <carter.li@eoitek.com>

This commit will: - Remove ppm, pgm, pgmyuv and tga from the Preferences.ScreenshotFormat enum - Add webp and jxl to the Preferences.ScreenshotFormat enum - Add WebP (.webp) and JPEG XL (.jxl) as choices for the Format setting in the Screenshots section of General settings - Add a new method createNSImageWithContentsOf to FFmpegController - Add a new private method createImage to PlayerCore that will call the createNSImageWithContentsOf method if the normal NSImage initializer is unable to decode the screenshot - Change PlayerCore.screenshotCallback to use the new method to create a NSImage to preview in the OSD - Add a new class FFmpegLogger that provides access to the normal IINA logger to the FFmpegController - Change the FFmpegController to use the IINA logger instead of NSLog when logging messages - Add an internal setting enableFFmpegImageDecoder to allow disabling use of the FFmpeg image decoder should a problem be uncovered - Change the screenshot preview animation duration to adhere to accessibility settings NOTE that removing enumeration cases breaks backward compatibility as the new cases reuse the tag values previously assigned to ppm and pgm. These four screenshot formats were dropped by mpv and removed from IINA's setting panel back in 2017. Anyone using these old formats would have had to update their settings therefore it should be acceptable to reuse the tag values. The main author of the settings changes is Carter Li. That portion of the PR is a merge of changes that have been tested in his IINA fork. The FFmpeg related changes are new and support the ability to show screenshot previews in the OSD for image formats not supported by AppKit. Currently JPEG XL is not supported by the NSImage initializer. This is also true for WebP in macOS versions before Big Sur. Co-authored-by: CarterLi <carter.li@eoitek.com>

This commit will: - Remove ppm, pgm, pgmyuv and tga from the Preferences.ScreenshotFormat enum - Add webp and jxl to the Preferences.ScreenshotFormat enum - Add WebP (.webp) and JPEG XL (.jxl) as choices for the Format setting in the Screenshots section of General settings - Add a new method createNSImageWithContentsOfURL to FFmpegController - Add a new private method createImage to PlayerCore that will call the createNSImageWithContentsOfURL method if the normal NSImage initializer is unable to decode the screenshot - Change PlayerCore.screenshotCallback to use the new method to create a NSImage to preview in the OSD - Add a new class FFmpegLogger that provides access to the normal IINA logger to the FFmpegController - Change the FFmpegController to use the IINA logger instead of NSLog when logging messages NOTE that removing enumeration cases breaks backward compatibility as the new cases reuse the tag values previously assigned to ppm and pgm. These four screenshot formats were dropped by mpv and removed from IINA's setting panel back in 2017. Anyone using these old formats would have had to update their settings therefore it should be acceptable to reuse the tag values. The main author of the settings changes is Carter Li. That portion of the PR is a merge of changes that have been tested in his IINA fork. The FFmpeg related changes are new and support the ability to show screenshot previews in the OSD for image formats not supported by AppKit. Currently JPEG XL is not supported by the NSImage initializer. This is also true for WebP in macOS versions before Big Sur. Co-authored-by: CarterLi <carter.li@eoitek.com>

low-batt · 2024-05-27T00:38:54Z

Rebased with develop and fixed merge conflicts.

lhc70000 · 2024-06-19T00:18:18Z

NOTE that removing enumeration cases breaks backward compatibility as the new cases reuse the tag values previously assigned to ppm and pgm.

Why not

  enum ScreenshotFormat: Int, InitializingFromKey {
    case png = 0
    case jpg
    case jpeg
    case webp = 7
    case jxl
}

low-batt · 2024-06-20T01:31:28Z

Back when this was first coded we decided that as these settings had been all dropped back in 2017 it was very unlikely that reusing the values would be a problem. If we want to avoid reusing then we can change the enum like shown. We'd also have to override the normal tag values in the XIB for the pull down menu items so the tags match up with the enum values. All very doable. Should I make that change?

lhc70000 · 2024-06-20T01:36:28Z

OK, it should be fine. If we also need to change the XIB then it's probably not worth the hassle.

low-batt marked this pull request as draft April 20, 2023 00:36

low-batt linked an issue Apr 20, 2023 that may be closed by this pull request

JPEG-XL decoding/screenshot support? #3852

Closed

low-batt force-pushed the close-3852 branch from 41cf3fa to 75213ba Compare May 25, 2023 22:32

low-batt marked this pull request as ready for review May 25, 2023 23:01

low-batt requested review from uiryuu, lhc70000 and saagarjha May 25, 2023 23:03

low-batt commented May 25, 2023

View reviewed changes

iina/FFmpegController.m Outdated Show resolved Hide resolved

saagarjha reviewed Jun 3, 2023

View reviewed changes

low-batt mentioned this pull request Aug 13, 2023

Support screenshots in AVIF format #4579

Open

low-batt mentioned this pull request Nov 24, 2023

JPEG-XL decoding/screenshot support? #3852

Closed

low-batt and others added 3 commits May 26, 2024 20:10

low-batt force-pushed the close-3852 branch from 60344aa to 4947dee Compare May 27, 2024 00:38

lhc70000 self-assigned this Jun 19, 2024

lhc70000 merged commit 2ed5d72 into develop Jun 20, 2024

lhc70000 deleted the close-3852 branch June 20, 2024 01:37

low-batt mentioned this pull request Jun 22, 2024

Fix data races, #3829, #3830, #3936, #3937 #4819

Merged

2 tasks

low-batt mentioned this pull request Feb 28, 2025

OSC thumbnails of HDR videos are clipped #5401

Open

3 tasks

	+ (nullable NSImage )createNSImageWithContentsOf:(nonnull NSURL )url;
	+ (nullable NSImage )createNSImageWithContentsOfURL:(nonnull NSURL )url;

Uh oh!

Close JPEG-XL decoding/screenshot support, #3852 #4367

Close JPEG-XL decoding/screenshot support, #3852 #4367

Uh oh!

Conversation

low-batt commented Apr 20, 2023

Uh oh!

low-batt commented Apr 20, 2023

Uh oh!

low-batt commented May 25, 2023

Uh oh!

Uh oh!

CarterLi commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

low-batt commented May 26, 2023

Uh oh!

low-batt commented May 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

low-batt commented May 27, 2024

Uh oh!

lhc70000 commented Jun 19, 2024

Uh oh!

low-batt commented Jun 20, 2024

Uh oh!

lhc70000 commented Jun 20, 2024

Uh oh!

Uh oh!

CarterLi commented May 26, 2023 •

edited

Loading