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

Wrong metadata handling when file has min: 0.0000 cd/m2, max: 1000 cd/m2 values #14177

Closed
6 tasks done
mitzsch opened this issue May 19, 2024 · 9 comments
Closed
6 tasks done

Comments

@mitzsch
Copy link
Contributor

mitzsch commented May 19, 2024

mpv Information

mpv v0.37.0-766-ge42a8d53 Copyright © 2000-2024 mpv/MPlayer/mplayer2 projects
 built on Apr 13 2024 00:07:59
libplacebo version: v7.349.0 (v6.338.0-121-ge4e096b-dirty)
FFmpeg version: N-114826-g65c1c83ca
FFmpeg library versions:
   libavutil       59.15.100
   libavcodec      61.5.102
   libavformat     61.3.100
   libswscale      8.2.100
   libavfilter     10.2.101
   libswresample   5.2.100

Other Information

  • Windows version: 10 22H2 / Win 11 23H2
  • GPU model, driver and version: RTX 3060/4060 - 552.44
  • Source of mpv: shinchiro / self-compiled
  • Introduced in version: mpv v0.37.0-766-ge42a8d53

Reproduction Steps

Introduced with mpv v0.37.0-766-ge42a8d53 because of presumably this ffmpeg commit FFmpeg/FFmpeg@1c45104. (I know its a ffmpeg issue but the commit is made by mpv devs*)
The commit causes metadata to be wrongly handled.

If the file has metadata values like 0.0000 cd/m2, max: 1000 cd/m2, the max value gets reported as 10.000 cd/m2.
=>
Screenshot 2024-05-19 185427

This is not only a reporting bug, the wrong max value is also reported to the display. (checked by a hdfury arcana)

The 0.0000 cd/m2 value for the min value triggers the wrong handling. Any other file with a non-0.0000 value, is treated correctly - the max value stays at 1000 cd/m2

mpv.com file.mkv -vo=gpu-next is enough to trigger this behavior, hit i and you will see the wrong metadata value

Expected Behavior

Do not "alter" the value, process them like they are encoded in the file.
file => 0.0000 cd/m2, max: 1000 cd/m2 => output => 0.0000 cd/m2, max: 1000 cd/m2

Actual Behavior

file => 0.0000 cd/m2, max: 1000 cd/m2 => output => 0.0000 cd/m2, max: 10.000 cd/m2

Log File

mpv_log_metadata_value.txt
mpv_log_metadata_value_upstream_mpv.txt

Sample Files

No response

I carefully read all instruction and confirm that I did the following:

  • I tested with the latest mpv version to validate that the issue is not already fixed.
  • I provided all required information including system and mpv version.
  • I produced the log file with the exact same set of files, parameters, and conditions used in "Reproduction Steps", with the addition of --log-file=output.txt.
  • I produced the log file while the behaviors described in "Actual Behavior" were actively observed.
  • I attached the full, untruncated log file.
  • I attached the backtrace in the case of a crash.
@mitzsch mitzsch added the os:win label May 19, 2024
@kasper93
Copy link
Contributor

If the file has metadata values like 0.0000 cd/m2

Then it is an invalid file, and there is nothing we can do about it.

@mitzsch
Copy link
Contributor Author

mitzsch commented May 19, 2024

Okay, but those values are found on commercially available discs. The 4K UK version of La La Land for example.
In passthrough mode (-target-colorspace-hint=yes), I would rather have this "wrong" value to be transmitted instead of an entirely wrong max value.

Edit:
The output looks wrong with the value of max 10.000 cd/m2 - with a "working" version of mpv it looks fine.

@kasper93
Copy link
Contributor

Specification is clear on this:

min_display_mastering_luminance, when in the range of 1 to 50 000, specifies the nominal minimum display luminance of the mastering display in units of 0.0001 candelas per square metre. When min_display_mastering_luminance is not in the range of 1 to 50 000, the nominal maximum display luminance of the mastering display is unknown or unspecified or specified by other means not specified in this Specification. When max_display_mastering_luminance is equal to 50 000, min_display_mastering_luminance shall not be equal to 50 000.

If the official distribution disc exist with 0 value, we can make an exception for this. It has to be validated, if this is more common issues or isolated incident.

@mitzsch
Copy link
Contributor Author

mitzsch commented May 19, 2024

Thanks for linking the specifications. However, there are multiple discs out there with min values of 0.0000 cd/m2 - Baywatch and The Martian in 4K are other examples.

we can make an exception for this.

This would be great, at least for "untouched" processing modes like passthrough it would be great if the values of the file could just be sent as is. For tone mapping done by libplacebo/mpv/ffmpeg it's probably fine to handle it differently - although I think forcing it to a max value of 10.000 creates wrong results....

@kasper93
Copy link
Contributor

@haasn: Would you be able to handle this case on ffmpeg side? I'm not sure sending patch myself would get reviewed anyway.

@mitzsch
Copy link
Contributor Author

mitzsch commented May 25, 2024

Anything new on this one? :)

@haasn
Copy link
Member

haasn commented May 25, 2024

https://ffmpeg.org//pipermail/ffmpeg-devel/2024-May/328170.html

@mitzsch
Copy link
Contributor Author

mitzsch commented May 25, 2024

Thanks!

BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 28, 2024
The H.265 specification is quite clear on this case:

> When min_display_mastering_luminance is not in the range of 1 to
> 50000, the nominal maximum display luminance of the mastering display
> is unknown or unspecified or specified by other means not specified in
> this Specification.

And so the current code is correct in marking luminance data as invalid
if min luminance is set to 0. However, this breaks playback of at least
several real-world Blu-ray releases, for example La La Land, Planet of
the Apes, and quite possibly a lot more. These come with ostensibly
valid max_luminance tags (1000 nits), but min_luminance set to 0.

Loosen up this requirement by guarding it behind FF_COMPLIANCE_STRICT.
We still reject blatantly invalid metadata (wrong value range on
luminance, max set to 0, max below min, min above 50 nits etc.), so this
shouldn't cause any unintended regressions.

Fixes: mpv-player/mpv#14177
@mitzsch
Copy link
Contributor Author

mitzsch commented May 28, 2024

Fixed in FFmpeg/FFmpeg@9fd88bd
Thanks!

@mitzsch mitzsch closed this as completed May 28, 2024
richardpl pushed a commit to librempeg/librempeg that referenced this issue May 31, 2024
The H.265 specification is quite clear on this case:

> When min_display_mastering_luminance is not in the range of 1 to
> 50000, the nominal maximum display luminance of the mastering display
> is unknown or unspecified or specified by other means not specified in
> this Specification.

And so the current code is correct in marking luminance data as invalid
if min luminance is set to 0. However, this breaks playback of at least
several real-world Blu-ray releases, for example La La Land, Planet of
the Apes, and quite possibly a lot more. These come with ostensibly
valid max_luminance tags (1000 nits), but min_luminance set to 0.

Loosen up this requirement by guarding it behind FF_COMPLIANCE_STRICT.
We still reject blatantly invalid metadata (wrong value range on
luminance, max set to 0, max below min, min above 50 nits etc.), so this
shouldn't cause any unintended regressions.

Fixes: mpv-player/mpv#14177
Signed-off-by: Paul B Mahol <onemda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants