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

make AlphaMode mandatory #558

Merged
merged 4 commits into from Jan 30, 2022
Merged

make AlphaMode mandatory #558

merged 4 commits into from Jan 30, 2022

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Sep 12, 2021

This is a WebM thing loosely defined. I'm not sure where the default value
came from.

Having just the presence of the element present to signal something is at odd
with all other elements. The default value in this case has no use. And
following the discussions in #501 elements with a default value would preferably
be mandatory. But that's not possible with the "presence" signaling something.

So just remove the default value.

Both libavformat [1] and libwebm [2] write a value of 1 when they want to signal
it.

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L1274
[2] https://aomedia.googlesource.com/aom/+/refs/heads/master/third_party/libwebm/mkvmuxer/mkvmuxer.cc#1522

@robUx4 robUx4 added bug format addition clarifications spec_main Main Matroska spec document target labels Sep 12, 2021
@JeromeMartinez
Copy link
Contributor

For me a default of 0 is fine, and actually better. 0 could be defined as "Unknown".

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 3, 2021

But that's not compatible with what the WebM spec says:

Alpha Video Mode. Presence of this element indicates that the BlockAdditional element could contain Alpha data.

0, 1, 2, ..., 666, 667, ... all mean that the BlockAdditional element could contain alpha data. That would break compatibility with existing parsers.

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 3, 2021

BTW Firefox, one of the browsers that can handle alpha in WebM, seems to use nestegg to parse WebM.
A value read as 0 doesn't mean the same as a value read as 1. The value of alpha_mode is 0 if the element is not present or if it was stored as 0.

https://github.com/mozilla/nestegg/blob/ec6adfbbf979678e3058cc4695257366f39e290b/src/nestegg.c#L2628
https://github.com/mozilla/nestegg/blob/ec6adfbbf979678e3058cc4695257366f39e290b/src/nestegg.c#L896

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 3, 2021

It seems to be the same in Chrome/chromium/Edge/etc. The internal read value is initialized to -1. And it's compared to 1 to determine whether the video is opaque (kIsOpaque) or has alpha (kHasAlpha).

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 3, 2021

In both cases, the presence of the element is not sufficient to determine if the video track has an alpha channel.

  • a value of 0 means it has no alpha channel,
  • a value of 1 means it has an alpha channel,
  • a value of 2, 3, etc means it has an alpha channel for Firefox, but not for Chromium.

We have 2 choices to match the rules discussed #501, remove the default value, or make it mandatory. In both cases we have to explain than only a value of 1 means there is an alpha channel.

I'd rather make it mandatory as done for all other elements with a default value of 0 in #557.

This is a WebM thing loosely defined.

Having just the presence of the element present to signal something is at odd
with all other elements. The default value in this case has no use. And
following the discussions in #501 elements with a default value would preferably
be mandatory. But that's not possible with the "presence" signaling something.

Both libavformat [1] and libwebm [2] write a value of 1 when they want to signal
it.

Chromium requires a value of 1 to consider the track has alpha [3].
Firefox consider there's no alpha value is 0 is stored [4].

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L1274
[2] https://aomedia.googlesource.com/aom/+/refs/heads/master/third_party/libwebm/mkvmuxer/mkvmuxer.cc#1522
[3] https://github.com/chromium/chromium/blob/72ceeed2ebcd505b8d8205ed7354e862b871995e/media/formats/webm/webm_video_client.cc#L149
[4] https://github.com/mozilla/nestegg/blob/ec6adfbbf979678e3058cc4695257366f39e290b/src/nestegg.c#L2627
This is how other flags are defined in the document.

Both libavformat [1] and libwebm [2] write a value of 1 when they want to signal
it. This is compatible with how Chromium (the reference) works.

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L1274
[2] https://aomedia.googlesource.com/aom/+/refs/heads/master/third_party/libwebm/mkvmuxer/mkvmuxer.cc#1522
@robUx4 robUx4 changed the title remove AlphaMode default value make AlphaMode mandatory Oct 3, 2021
@robUx4
Copy link
Contributor Author

robUx4 commented Oct 3, 2021

Make mandatory as in #557 and define it as other flags (value 1 means defined).

The BlockAddID of 1 must be considered as alpha data when the AlphaMode flag is set.

That's how it's written by libavformat (only 1 is considered and it's alpha value) [1]

[1] https://github.com/FFmpeg/FFmpeg/blob/e26c4d252faff4f7b1bf3087f609b699f20b47d3/libavcodec/libvpxdec.c#L238
ebml_matroska.xml Outdated Show resolved Hide resolved
@robUx4 robUx4 merged commit 61543af into master Jan 30, 2022
@robUx4 robUx4 deleted the no-default-alpha branch January 30, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants