[RFC] Reapply "demux_mkv: PAR should be calculated after applying crop"#13446
[RFC] Reapply "demux_mkv: PAR should be calculated after applying crop"#13446kasper93 wants to merge 1 commit intompv-player:masterfrom
Conversation
Follow the spec for better or for worse. This reverts commit 81102b0.
|
Well, if we want to follow the spec, this is the commit. The question is how many files follow the spec and if we need a compat option to handle broken files? Muxing a file with MKVToolNix: Which is not expected IMHO and maybe it is my fault for not understanding how to use MKVToolNix. But I feel like most if not all users would not know how to set the DisplayWidth/DisplayHeight, which is not excuse, but I'm just trying to make the least people mad. (Also there is no way to not set DisplayWidth/DisplayHeight in MKVToolNix, which would be another solution ofc) EDIT: Maybe I should ask smarter people than me how we suppose to handle this. @mbunkus would you help us out? |
|
My thoughts: Matroska spec states:
When I read "Applies to the video frame after cropping" I believe it means "Applies (the PAR transform) to the video frame after cropping". This makes sense, but the matroska spec never seems to specify whether the PAR should be calculated using the original dimensions, or the cropped dimensions. This key detail being left out has lead to confusion here with how this feature should actually be implemented. Despite the spec leaving this detail out, I believe we can make a perfectly reasonable assumption on how PAR is to be calculated. My opinion is that PAR should always be calculated using the original image dimensions, here is why:
It would be nice if the spec was clear on this subject, and I suppose this can also serve as a pitch for what the spec should be. 🙂 |
|
If you assume it talks about applying some particular, fixed linear transform, then it wouldn’t matter whatsoever when this transform is applied. To me, it very clearly says that the picture, after cropping, is to be rescaled to have the size specified by DisplayWidth/Height. In fact, if you follow the letter of the spec, then DisplayUnit=0 says DisplayWidth/Height are absolutely exact pixel dimensions that you must rescale the picture to. But even if DisplayUnit is something else or you ignore this stipulation, DisplayWidth divided by DisplayHeight still unambiguously defines the aspect ratio of the resulting picture. I genuinely fail to see how else this can be interpreted. See also the section on cropping:
and the defaults for
|
|
In the meantime I found related discussion here https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389
This is exactly my main issue. I don't want to make mpv become not compatible with seemingly all matroska files with cropping metadata. I think cropping should not change the pixel aspect ratio of the target rect. Which would makes files compatible with both crop-aware and crop-unaware players. That's why the commit was reverted to fix the playback of existing matroska files, which most of them from what I've seen are setting DisplayWidth/Height to uncropped values. That's also why I pinged @mbunkus, because I would like to have authoritative statement on cropping behavior. So that we can refer to that if users complain about "broken" files. Also as you know we can just make matroska cropping disabled behind a configuration flag, but I would really like to avoid that, because it is merely workaround and forces user to control something that should be done automatically be the player. |
|
To recap what I said elsewhere: As it stands now, I think the best solution would be to change the spec. Of course, we’ll need co-operation from spec maintainers for this. But if spec maintainers should insist otherwise, then I don’t like the prospect of making mpv consciously violate the spec. I think the original intention of Matroska spec writers, as well as the intention and meaning of the current spec text, is that crop should be very hard crop that happens before anything else including any aspect ratio calculations, and that the non-cropped size shouldn’t be exposed or used during rendering in any way other than to apply the crop. This is indeed how Haali originally implemented it, barring some conflict between Matroska crop and bitstream-embedded crop. This is also how I would define container crop myself if I were adding it as a new container feature today. But the problem is compatibility with software that makes no attempt to support Matroska crop. The spec says crop merely “should” be supported. Most players ignore Matroska crop and apply display aspect ratio to the video rectangle spit out by the decoder (nominally described by PixelWidth/PixelHeight—to be clear, this is the rectangle after the decoder has already applied bitstream-embedded crop). On the muxing side, too, MKVToolNix has apparently been writing values where DisplayWidth/Height match PixelWidth/Height without subtracting the crop offsets, and people have apparently been producing real files containing such crops. Further, if we assume that those people have been targeting a real player, then this means that somewhere out there there is a player other than mpv that actually supports crop—and it treats it the same way as MKVToolNix. |
|
Oh, one thing that will need to be considered and made abundantly clear in the wording if the spec is changed is whether the crop values refer to pre-stretching pixels or post-stretching. @wiwaz Could this be what you meant earlier by any chance? That is, this is clear: But this is less clear: Is this |
Not exactly, the spec does make it explicitly clear that cropping is always applied before resizing the image according to DisplayWidth/DisplayHeight
The latter, your former example does not adhere to the spec. The concern I brought up is whether the amount the image should be stretched by is calculated using the original dimensions, or the cropped dimensions. The spec has no information on which is to be used, so I've laid out why always using the original dimensions should be the desired behavior. |
I may have phrased the question poorly, but sounds like a yes to me: that is, you think that what the spec does define is exactly this.
Of course. To the current spec. Neither example does, because the spec does define what its transform is: it transforms “the video frame after cropping” (in your own quote above) to the dimensions specified by DisplayWidth/Height, that is, |
|
Worth adding that according to matroska.org, crop is also part of WebM, which sounds like it might have a bigger impact beyond the bunch of old-fashioned anime fans that we are. But from a few quick tests, it seems Blink completely ignores the crop properties and applies DisplayWidth/Height to the whole encoded video (matching the hypothetical adjusted spec wording), whereas Gecko does something weird. I’m not quite sure, but I think Gecko’s support for crop is just broken and is worse than no support. Depending on the aspect ratios, it either crops nothing or crops various amounts off the bottom even when my crop says Top. I haven’t been able to deduce a pattern. I’ve only tested these two engines. |
|
After thinking about it, discussing and testing with samples that I have I decide to close this PR. I think we can agree that the current Matroska specification is not perfect. I agree that we should aim to amend it to resolve current issues with cropping. I also tested multiple samples that I have found and they are not only "anime", I gathered some archival stuff from archive.org, they surprisingly have crop parameters in some of the videos. It has been confirmed that MKVToolNix currently does not consider crop values when calculating default DisplayWidth/DisplayHeight values. Which effectively produces non-spec compliant files, unless user manually calculate the correct values. But I honestly think, no one is doing that, as it is even not that trivial to calculate the value... Given that if we would follow current version of specification to the letter, mpv would play most if not all files incorrectly. That's why the conclusion is the same as before and the commit in question will be kept reverted. We can still discuss here, but I suggest consolidating discussion over here https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 I will also try to summarize the issue in short email to IETF ML as suggested, for visibility and possibly to kick-start discussion on updating the spec. |
|
I don't deny the advantages of applying PAR first but then I really hope that the spec will be changed to match it.
I also feared that cropping was applied on a resized (by DisplayWidth/DisplayHeight) image which would be an issue in several ways. Good to see that the current implementation crops the original pixels. In fact, the whole issue comes from the existence of DisplayWidth/DisplayHeight properties: there should be only PAR properties.
Apart from that I'm curious what other software supports MKV cropping. I see none mentioned but they should exist if there already are cropped videos in the wild.
|
|
I do However, when cropping from the command line, one must now first find the DAR value with FFmpeg, fire up "Calculator" to crunch on "876 * DAR", and then change the command line accordingly. Not so funny... Just weighing in on an aspect of the change for future considerations. PS! I noticed that everyone refers to Matroska files, but the same happens with MP4. So probably related to interchangeable metadata rather than a singular container format. |
The primary motivation for this change is to ensure that Matroska files with `PixelCrop*` elements are displayed consistently, regardless of whether cropping is applied. This makes such files compatible across software that either supports or ignores these elements. Without this change, files suffer from limited compatibility, whereas cropping tags should enhance the experience rather than be a requirement for proper image display. A secondary motivation is that an overwhelming number of files in the wild do not actually adjust the `DisplayWidth`/`DisplayHeight` elements as expected by the specification. Most of the time, these values are left uncropped, with the expectation that cropping will be applied later. This issue arises partly because MKVToolNix does not automatically perform this calculation, it only sets the `PixelCrop*` elements unless explicitly instructed by the user. Observing the existing files in circulation, it is evident that almost no one manually adjusts these values. For a more detailed discussion on this topic, please refer to the links below: See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 See: mpv-player/mpv#13446
The primary motivation for this change is to ensure that Matroska files with `PixelCrop*` elements are displayed consistently, regardless of whether cropping is applied. This makes such files compatible across software that either supports or ignores these elements. Without this change, files suffer from limited compatibility, whereas cropping tags should enhance the experience rather than be a requirement for proper image display. A secondary motivation is that an overwhelming number of files in the wild do not actually adjust the `DisplayWidth`/`DisplayHeight` elements as expected by the specification. Most of the time, these values are left uncropped, with the expectation that cropping will be applied later. This issue arises partly because MKVToolNix does not automatically perform this calculation, it only sets the `PixelCrop*` elements unless explicitly instructed by the user. Observing the existing files in circulation, it is evident that almost no one manually adjusts these values. Lastly, some subtitle formats depend on the video size for positioning and rendering, making them sensitive to cropping. Currently, most authoring tools do not take cropping into account. By making the Matroska specification more flexible and allowing subtitles to be cropped along with the video, it becomes easier to produce files that are compatible with various Matroska players. For a more detailed discussion on this topic, please refer to the links below: See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 See: mpv-player/mpv#13446
The primary motivation for this change is to ensure that Matroska files with `PixelCrop*` elements are displayed consistently, regardless of whether cropping is applied. This makes such files compatible across software that either supports or ignores these elements. Without this change, files suffer from limited compatibility, whereas cropping tags should enhance the experience rather than be a requirement for proper image display. A secondary motivation is that an overwhelming number of files in the wild do not actually adjust the `DisplayWidth`/`DisplayHeight` elements as expected by the specification. Most of the time, these values are left uncropped, with the expectation that cropping will be applied later. This issue arises partly because MKVToolNix does not automatically perform this calculation, it only sets the `PixelCrop*` elements unless explicitly instructed by the user. Observing the existing files in circulation, it is evident that almost no one manually adjusts these values. Lastly, some subtitle formats depend on the video size for positioning and rendering, making them sensitive to cropping. Currently, most authoring tools do not take cropping into account. By making the Matroska specification more flexible and allowing subtitles to be cropped along with the video, it becomes easier to produce files that are compatible with various Matroska players. For a more detailed discussion on this topic, please refer to the links below: See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 See: mpv-player/mpv#13446
The primary motivation for this change is to ensure that Matroska files with `PixelCrop*` elements are displayed consistently, regardless of whether cropping is applied. This makes such files compatible across software that either supports or ignores these elements. Without this change, files suffer from limited compatibility, whereas cropping tags should enhance the experience rather than be a requirement for proper image display. A secondary motivation is that an overwhelming number of files in the wild do not actually adjust the `DisplayWidth`/`DisplayHeight` elements as expected by the specification. Most of the time, these values are left uncropped, with the expectation that cropping will be applied later. This issue arises partly because MKVToolNix does not automatically perform this calculation, it only sets the `PixelCrop*` elements unless explicitly instructed by the user. Observing the existing files in circulation, it is evident that almost no one manually adjusts these values. Lastly, some subtitle formats depend on the video size for positioning and rendering, making them sensitive to cropping. Currently, most authoring tools do not take cropping into account. By making the Matroska specification more flexible and allowing subtitles to be cropped along with the video, it becomes easier to produce files that are compatible with various Matroska players. For a more detailed discussion on this topic, please refer to the links below: See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 See: mpv-player/mpv#13446
The primary motivation for this change is to ensure that Matroska files with `PixelCrop*` elements are displayed consistently, regardless of whether cropping is applied. This makes such files compatible across software that either supports or ignores these elements. Without this change, files suffer from limited compatibility, whereas cropping tags should enhance the experience rather than be a requirement for proper image display. A secondary motivation is that an overwhelming number of files in the wild do not actually adjust the `DisplayWidth`/`DisplayHeight` elements as expected by the specification. Most of the time, these values are left uncropped, with the expectation that cropping will be applied later. This issue arises partly because MKVToolNix does not automatically perform this calculation, it only sets the `PixelCrop*` elements unless explicitly instructed by the user. Observing the existing files in circulation, it is evident that almost no one manually adjusts these values. Lastly, some subtitle formats depend on the video size for positioning and rendering, making them sensitive to cropping. Currently, most authoring tools do not take cropping into account. By making the Matroska specification more flexible and allowing subtitles to be cropped along with the video, it becomes easier to produce files that are compatible with various Matroska players. For a more detailed discussion on this topic, please refer to the links below: See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 See: mpv-player/mpv#13446
I don't expect the Matroska specification to be adjusted for this to work soon, so in the meantime, follow the spec as written and add an option to support most files in the wild. I would prefer this to never be an option, but it looks like it's unavoidable. Recently, FFmpeg added cropping support for Matroska, so we don’t want to be the odd one out by doing it differently... See for more details: ietf-wg-cellar/matroska-specification#947 https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 mpv-player#13446 Fixes: mpv-player#15800
I don't expect the Matroska specification to be adjusted for this to work soon, so in the meantime, follow the spec as written and add an option to support most files in the wild. I would prefer this to never be an option, but it looks like it's unavoidable. Recently, FFmpeg added cropping support for Matroska, so we don’t want to be the odd one out by doing it differently... See for more details: ietf-wg-cellar/matroska-specification#947 https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 mpv-player#13446 Fixes: mpv-player#15800
I don't expect the Matroska specification to be adjusted for this to work soon, so in the meantime, follow the spec as written and add an option to support most files in the wild. I would prefer this to never be an option, but it looks like it's unavoidable. Recently, FFmpeg added cropping support for Matroska, so we don’t want to be the odd one out by doing it differently... See for more details: ietf-wg-cellar/matroska-specification#947 https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 #13446 Fixes: #15800
I don't expect the Matroska specification to be adjusted for this to work soon, so in the meantime, follow the spec as written and add an option to support most files in the wild. I would prefer this to never be an option, but it looks like it's unavoidable. Recently, FFmpeg added cropping support for Matroska, so we don’t want to be the odd one out by doing it differently... See for more details: ietf-wg-cellar/matroska-specification#947 https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 mpv-player#13446 Fixes: mpv-player#15800
I don't expect the Matroska specification to be adjusted for this to work soon, so in the meantime, follow the spec as written and add an option to support most files in the wild. I would prefer this to never be an option, but it looks like it's unavoidable. Recently, FFmpeg added cropping support for Matroska, so we don’t want to be the odd one out by doing it differently... See for more details: ietf-wg-cellar/matroska-specification#947 https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389 mpv-player#13446 Fixes: mpv-player#15800
Currently, Matroska allows a video's sample aspect ratio to be set via
the display aspect ratio through the `DisplayWidth`/`DisplayHeight`
elements.
However, these elements are currently specified to encode the display
aspect ratio *after* applying cropping. This creates inconsistencies
with players that do not read the `PixelCrop` elements and, in theory,
makes it impossible for Matroska writers to create files that display
with the correct aspect ratio both in players that respect cropping and
in players that do not.
Moreover, many existing files do not actually follow this convention
that the display aspect ratio apply to the cropped video, and instead
store the display aspect ratio of the uncropped video. In particular,
MKVToolNix stores the `DisplayWidth`/`DisplayHeight` in this format.
In order to be able to resolve these discrepancies in the future
without changing existing behavior, this commit adds explicit
`SampleAspectRatio{Num,Den}` elements, which specify the sample aspect
ratio rather than the display aspect ratio. This has the advantage of
being independent of any cropping and is also consistent with the way
the sample/display aspect ratios are stored in video coding formats
like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio
indicator"). In cases where the `DisplayWidth`/`DisplayHeight` and the
`SampleAspectRatioNum`/`SampleAspectRatioDen` elements are both present
and might conflict, it is clearly specified in what way the sample
aspect ratio elements take precedence. Matroska writers can write both
kinds of elements without risking conflicts.
Refer to the following links for previous discussion about these issues.
See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389
See: mpv-player/mpv#13446
Currently, Matroska allows a video's sample aspect ratio to be set via
the display aspect ratio through the `DisplayWidth`/`DisplayHeight`
elements.
However, these elements are currently specified to encode the display
aspect ratio *after* applying cropping. This creates inconsistencies
with players that do not read the `PixelCrop` elements and, in theory,
makes it impossible for Matroska writers to create files that display
with the correct aspect ratio both in players that respect cropping and
in players that do not.
Moreover, many existing files do not actually follow this convention
that the display aspect ratio apply to the cropped video, and instead
store the display aspect ratio of the uncropped video. In particular,
MKVToolNix stores the `DisplayWidth`/`DisplayHeight` in this format.
In order to be able to resolve these discrepancies in the future
without changing existing behavior, this commit adds explicit
`SampleAspectRatio{Num,Den}` elements, which specify the sample aspect
ratio rather than the display aspect ratio. This has the advantage of
being independent of any cropping and is also consistent with the way
the sample/display aspect ratios are stored in video coding formats
like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio
indicator"). In cases where the `DisplayWidth`/`DisplayHeight` and the
`SampleAspectRatioNum`/`SampleAspectRatioDen` elements are both present
and might conflict, it is clearly specified in what way the sample
aspect ratio elements take precedence. Matroska writers can write both
kinds of elements without risking conflicts.
Refer to the following links for previous discussion about these issues.
See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389
See: mpv-player/mpv#13446
Currently, Matroska allows a video's sample aspect ratio to be set via
the display aspect ratio through the `DisplayWidth`/`DisplayHeight`
elements.
However, these elements are currently specified to encode the display
aspect ratio *after* applying cropping. This creates inconsistencies
with players that do not read the `PixelCrop` elements and, in theory,
makes it impossible for Matroska writers to create files that display
with the correct aspect ratio both in players that respect cropping and
in players that do not.
Moreover, many existing files do not actually follow this convention
that the display aspect ratio apply to the cropped video, and instead
store the display aspect ratio of the uncropped video. In particular,
MKVToolNix stores the `DisplayWidth`/`DisplayHeight` in this format.
In order to be able to resolve these discrepancies in the future
without changing existing behavior, this commit adds explicit
`SampleAspectRatio{Num,Den}` elements, which specify the sample aspect
ratio rather than the display aspect ratio. This has the advantage of
being independent of any cropping and is also consistent with the way
the sample/display aspect ratios are stored in video coding formats
like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio
indicator"). In cases where the `DisplayWidth`/`DisplayHeight` and the
`SampleAspectRatioNum`/`SampleAspectRatioDen` elements are both present
and might conflict, it is clearly specified in what way the sample
aspect ratio elements take precedence. Matroska writers can write both
kinds of elements without risking conflicts.
Refer to the following links for previous discussion about these issues.
See: https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389
See: mpv-player/mpv#13446
Currently, Matroska allows a video's sample aspect ratio to be set via
the display aspect ratio through the `DisplayWidth`/`DisplayHeight`
elements.
However, these elements are currently specified to encode the display
aspect ratio *after* applying cropping. This creates inconsistencies
with players that do not read the `PixelCrop` elements and, in theory,
makes it impossible for Matroska writers to create files that display
with the correct aspect ratio both in players that respect cropping and
in players that do not.
Moreover, many existing files do not actually follow this convention
that the display aspect ratio apply to the cropped video, and instead
store the display aspect ratio of the uncropped video. In particular,
MKVToolNix stores the `DisplayWidth`/`DisplayHeight` in this format.
In order to be able to resolve these discrepancies in the future
without changing existing behavior, this commit adds explicit
`SampleAspectRatio{Num,Den}` elements, which specify the sample aspect
ratio rather than the display aspect ratio. This has the advantage of
being independent of any cropping and is also consistent with the way
the sample/display aspect ratios are stored in video coding formats
like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio
indicator"). In cases where the `DisplayWidth`/`DisplayHeight` and the
`SampleAspectRatioNum`/`SampleAspectRatioDen` elements are both present
and might conflict, it is clearly specified in what way the sample
aspect ratio elements take precedence. Matroska writers can write both
kinds of elements without risking conflicts.
Refer to the following links for previous discussion about these issues.
See: https://codeberg.org/mbunkus/mkvtoolnix/issues/2389
See: mpv-player/mpv#13446
Follow the spec for better or for worse.
This reverts commit 81102b0.