This repository has been archived by the owner. It is now read-only.

support editing/adding colour elements in header editor #2038

Closed
dericed opened this Issue Jul 11, 2017 · 16 comments

Comments

5 participants
@dericed

dericed commented Jul 11, 2017

No description provided.

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol commented Jul 26, 2017

👍

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol commented Jul 26, 2017

Might as well link to the conversation on Cellar as well:https://mailarchive.ietf.org/arch/msg/cellar/khsmYeogAVnBioQbzabSTZydGus

mbunkus added a commit that referenced this issue Aug 3, 2017

propedit: add general support for elements in (sub-)sub-sub-masters
This is needed for implementing #2038 and #2064. Those new elements
located in `Track → TrackVideo → VideoColour` and `Track → TrackVideo
→ VideoColour → VideoColourMasteringMeta`, meaning four levels
deep. So far both mkvpropedit and the GUI's header editor only
supported two levels (`Track → TrackVideo` and `Track → TrackAudio`).

mbunkus added a commit that referenced this issue Aug 3, 2017

propedit: add support for video colour attributes
This implements one half of #2038.

Note that this commit breaks the GUI's header editor until support for
editing sub-sub-sub-masters will be added to it (soon).

mbunkus added a commit that referenced this issue Aug 5, 2017

GUI: headers: refactor page initialization to use `property_element_c`
The information about each element's parents, its type, title and
description is available in property_element_c and used by mkvpropedit
already. As that's all the information needed by the GUI's header
editor, there's no reason to duplicate it in the header
editor. Instead it can iterate over the tables provided by
`property_element_c` and create pages based on that information.

This is in preparation of supporting the changes required for #2038
and #2064.

mbunkus added a commit that referenced this issue Aug 5, 2017

GUI: headers: support for editing video colour track attributes
Implements the second half of #2038. Also adds the necessary changes
for implementing #2064 eventually.

@mbunkus mbunkus closed this Aug 5, 2017

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

I've uploaded new pre-builds for Windows that include the functionality. I'd appreciate some testing.

Owner

mbunkus commented Aug 5, 2017

I've uploaded new pre-builds for Windows that include the functionality. I'd appreciate some testing.

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol Aug 5, 2017

kieranjol commented Aug 5, 2017

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol Aug 5, 2017

Hi Moritz,

I was able to add the extra metadata just fine with
mkvpropedit.exe --edit track:v1 --set colour-primaries=5 --set colour-range=1 --set colour-matrix=5 --set colour-transfer-characteristics=3 oe5909_vrep_clip.mkv
using https://drive.google.com/open?id=0B80EiSambHhLSnMwQnc1NFZ0Mnc
as my source file. Thanks! This will make life a lot easier for us in the Irish Film Institute as we can add this metadata to several hundred files without creating new versions. The resulting file now passes the Mediaconch 'is Matroska well described policy' .I have some feedback though;

  • According to mediaconch, the file that was generated is not a valid Matroska file. It seems the element in question is:
<test outcome="fail" reason="Timecode MUST be a Child Element of Cluster but is not present.">
          <value name="Cluster" offset="1568" context="/Segment[1]/Cluster[1]" formatid="0x1F43B675">[472989 bytes]</value>
        </test>

Full report here: https://gist.github.com/kieranjol/cd233abae018ceb05c0cd936a1f3371c
Output file here: https://drive.google.com/file/d/0ByvCvU9YVB09WTNSQ2ZEdXk2YXM/view?usp=sharing

  • I see that the colour-matrix option doesn't use the terminology in the Matroska spec, which is MatrixCoefficients

That's pretty much it from me, thanks again Moritz.

kieranjol commented Aug 5, 2017

Hi Moritz,

I was able to add the extra metadata just fine with
mkvpropedit.exe --edit track:v1 --set colour-primaries=5 --set colour-range=1 --set colour-matrix=5 --set colour-transfer-characteristics=3 oe5909_vrep_clip.mkv
using https://drive.google.com/open?id=0B80EiSambHhLSnMwQnc1NFZ0Mnc
as my source file. Thanks! This will make life a lot easier for us in the Irish Film Institute as we can add this metadata to several hundred files without creating new versions. The resulting file now passes the Mediaconch 'is Matroska well described policy' .I have some feedback though;

  • According to mediaconch, the file that was generated is not a valid Matroska file. It seems the element in question is:
<test outcome="fail" reason="Timecode MUST be a Child Element of Cluster but is not present.">
          <value name="Cluster" offset="1568" context="/Segment[1]/Cluster[1]" formatid="0x1F43B675">[472989 bytes]</value>
        </test>

Full report here: https://gist.github.com/kieranjol/cd233abae018ceb05c0cd936a1f3371c
Output file here: https://drive.google.com/file/d/0ByvCvU9YVB09WTNSQ2ZEdXk2YXM/view?usp=sharing

  • I see that the colour-matrix option doesn't use the terminology in the Matroska spec, which is MatrixCoefficients

That's pretty much it from me, thanks again Moritz.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

Thanks for the feedback.

According to mediaconch, the file that was generated is not a valid Matroska file. It seems the element in question is:

The file wasn't created by mkvmerge but by libavf which is libavformat from ffmpeg or something using libavformat if I'm not mistaken (see mkvinfo's output regarding "multiplexing application" and "writing application". mkvpropedit only modifies the track headers but not the clusters. Therefore this isn't my bug, but ffmpeg's.

I see that the colour-matrix option doesn't use the terminology in the Matroska spec, which is MatrixCoefficients

That is… huh… that's an accident. I'll change the option's name to matrix-coefficients.

Basically my naming scheme is to take the last component from the element's path attribute and to convert it to using dashes instead of capitalization. This has obviously gone wrong for MatrixCoefficients.

Owner

mbunkus commented Aug 5, 2017

Thanks for the feedback.

According to mediaconch, the file that was generated is not a valid Matroska file. It seems the element in question is:

The file wasn't created by mkvmerge but by libavf which is libavformat from ffmpeg or something using libavformat if I'm not mistaken (see mkvinfo's output regarding "multiplexing application" and "writing application". mkvpropedit only modifies the track headers but not the clusters. Therefore this isn't my bug, but ffmpeg's.

I see that the colour-matrix option doesn't use the terminology in the Matroska spec, which is MatrixCoefficients

That is… huh… that's an accident. I'll change the option's name to matrix-coefficients.

Basically my naming scheme is to take the last component from the element's path attribute and to convert it to using dashes instead of capitalization. This has obviously gone wrong for MatrixCoefficients.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

Hmm now I know why I've used colour-matrix. The C++ class name is KaxColourMatrix, and I seem to have derived it from the class name. I'll change it to colour-matrix-coefficients. I'm using the colour- prefix for several properties in order to make them less ambiguous and future-proof (e.g. I cannot simply use bits-per-channel as that would clash with audio tracks, obviously).

Owner

mbunkus commented Aug 5, 2017

Hmm now I know why I've used colour-matrix. The C++ class name is KaxColourMatrix, and I seem to have derived it from the class name. I'll change it to colour-matrix-coefficients. I'm using the colour- prefix for several properties in order to make them less ambiguous and future-proof (e.g. I cannot simply use bits-per-channel as that would clash with audio tracks, obviously).

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol Aug 5, 2017

Thanks for getting back so quick.

It was indeed made by ffmpeg, however the source file is valid:
https://drive.google.com/open?id=0B80EiSambHhLSnMwQnc1NFZ0Mnc

but the file that was edited by mkvpropedit is not:
https://drive.google.com/file/d/0ByvCvU9YVB09WTNSQ2ZEdXk2YXM/view?usp=sharing

so i was thinking that it must have been something about the mkvpropedit process that caused the issue?

Also colour-matrix-coefficients sounds perfect, consistent with the other elements.

kieranjol commented Aug 5, 2017

Thanks for getting back so quick.

It was indeed made by ffmpeg, however the source file is valid:
https://drive.google.com/open?id=0B80EiSambHhLSnMwQnc1NFZ0Mnc

but the file that was edited by mkvpropedit is not:
https://drive.google.com/file/d/0ByvCvU9YVB09WTNSQ2ZEdXk2YXM/view?usp=sharing

so i was thinking that it must have been something about the mkvpropedit process that caused the issue?

Also colour-matrix-coefficients sounds perfect, consistent with the other elements.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

Meh, the option added to mkvmerge is called --colour-matrix, and that's where I took option names from, too… That code was contributed by someone else. Guess I should change it as well.

About changing that file. Well, I can guarantee you that mkvpropedit is not touching any of the clusters. And if you run mkvinfo -v -v mkvpropeditoutput.mkv, you'll see that there is a cluster timecode element in the cluster at 1568:

|+ Cluster at 1568
| + Cluster timecode: 0.000s at 1586
| + SimpleBlock (key, track number 1, 1 frame(s), timecode 0.000s = 00:00:00.000) at 1589
|  + Frame with size 472960

One thing that is different is that mkvpropedit had to relocate the track headers (the Tracks element) to the end of the file due to there not being enough space at the front. Of course that Tracks element is now referenced from the Meta Seek element at the start, so any compliant reader should be able to find the Tracks element just fine. But maybe MediaConch has some problems with that? See also this FAQ entry that describes this in more detail.

So I'm really not sure what MediaConch is complaining about, especially why it's emitting that particular error. You should probably check back with their developers.

Owner

mbunkus commented Aug 5, 2017

Meh, the option added to mkvmerge is called --colour-matrix, and that's where I took option names from, too… That code was contributed by someone else. Guess I should change it as well.

About changing that file. Well, I can guarantee you that mkvpropedit is not touching any of the clusters. And if you run mkvinfo -v -v mkvpropeditoutput.mkv, you'll see that there is a cluster timecode element in the cluster at 1568:

|+ Cluster at 1568
| + Cluster timecode: 0.000s at 1586
| + SimpleBlock (key, track number 1, 1 frame(s), timecode 0.000s = 00:00:00.000) at 1589
|  + Frame with size 472960

One thing that is different is that mkvpropedit had to relocate the track headers (the Tracks element) to the end of the file due to there not being enough space at the front. Of course that Tracks element is now referenced from the Meta Seek element at the start, so any compliant reader should be able to find the Tracks element just fine. But maybe MediaConch has some problems with that? See also this FAQ entry that describes this in more detail.

So I'm really not sure what MediaConch is complaining about, especially why it's emitting that particular error. You should probably check back with their developers.

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol Aug 5, 2017

Thank you so much. This has gone beyond my reckoning, so I'll link/copy paste your reply as a bug report in Mediaconch. Great work Moritz.

kieranjol commented Aug 5, 2017

Thank you so much. This has gone beyond my reckoning, so I'll link/copy paste your reply as a bug report in Mediaconch. Great work Moritz.

mbunkus added a commit that referenced this issue Aug 5, 2017

propedit: change `colour-matrix` property to `colour-matrix-coefficie…
…nts`

That's more in line with the specs, and it's clearer for the user what
this is actually about. See #2038.

mbunkus added a commit that referenced this issue Aug 5, 2017

merge: change `--colour-matrix` option to `--colour-matrix-coefficients`
That's more in line with the specs, and it's clearer for the user what
this is actually about. See #2038.
@JeromeMartinez

This comment has been minimized.

Show comment
Hide comment
@JeromeMartinez

JeromeMartinez Aug 5, 2017

So I'm really not sure what MediaConch is complaining about, especially why it's emitting that particular error.

Definitely a misleading error (the Cluster parsing is disabled in the library when it is before Tracks so the timecode is not parsed then the conformance checker sees that the timecode is not there because it is actually not parsed :(, so wrong error raised).
I fixed that part, no more error now and I am working on supporting file having Tracks after Cluster (for the moment I am supporting files which have no need of file seek support i.e. Tracks before Cluster, I'll add a tag saying that the file is not streamable when Tracks is after Cluster like I do for MOV).

You may consider a feature similar to ffmpeg -movflags faststart in order to make the file streamable (i.e. no need of seek) again by having the Track element again at the beginning of the file.

JeromeMartinez commented Aug 5, 2017

So I'm really not sure what MediaConch is complaining about, especially why it's emitting that particular error.

Definitely a misleading error (the Cluster parsing is disabled in the library when it is before Tracks so the timecode is not parsed then the conformance checker sees that the timecode is not there because it is actually not parsed :(, so wrong error raised).
I fixed that part, no more error now and I am working on supporting file having Tracks after Cluster (for the moment I am supporting files which have no need of file seek support i.e. Tracks before Cluster, I'll add a tag saying that the file is not streamable when Tracks is after Cluster like I do for MOV).

You may consider a feature similar to ffmpeg -movflags faststart in order to make the file streamable (i.e. no need of seek) again by having the Track element again at the beginning of the file.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

Thank you, Jerome, that sounds absolutely plausible.

There is no such flag in MKVToolNix:

  1. mkvmerge always produces files where the tracks are located in front of the clusters; hence it doesn't need such a flag.
  2. mkvpropedit cannot guarantee that it's always possible to place updated tracks before the clusters as several muxers (including ffmpeg's libavformat and HandBrake if I remember correctly) don't leave any extra space between tracks and clusters (mkvmerge always writes a ~1KB void element there in order to facilitate track header editing without having to relocate the tracks when they grow). The only way to guarantee that would be to rewrite pretty much everything if the tracks grow beyond how much space there is for them at the start of the file — and in that case remuxing with mkvmerge is better anyway.

I may think about a flag to mkvpropedit that aborts file modification if track headers cannot be placed at the start if that's something users might find useful. In such a case the file wouldn't be modified, and it would be left to the user to decide what to do.

Owner

mbunkus commented Aug 5, 2017

Thank you, Jerome, that sounds absolutely plausible.

There is no such flag in MKVToolNix:

  1. mkvmerge always produces files where the tracks are located in front of the clusters; hence it doesn't need such a flag.
  2. mkvpropedit cannot guarantee that it's always possible to place updated tracks before the clusters as several muxers (including ffmpeg's libavformat and HandBrake if I remember correctly) don't leave any extra space between tracks and clusters (mkvmerge always writes a ~1KB void element there in order to facilitate track header editing without having to relocate the tracks when they grow). The only way to guarantee that would be to rewrite pretty much everything if the tracks grow beyond how much space there is for them at the start of the file — and in that case remuxing with mkvmerge is better anyway.

I may think about a flag to mkvpropedit that aborts file modification if track headers cannot be placed at the start if that's something users might find useful. In such a case the file wouldn't be modified, and it would be left to the user to decide what to do.

@kieranjol

This comment has been minimized.

Show comment
Hide comment
@kieranjol

kieranjol Aug 5, 2017

Does mkclean do this kind of re-ordering? https://www.matroska.org/downloads/mkclean.html

kieranjol commented Aug 5, 2017

Does mkclean do this kind of re-ordering? https://www.matroska.org/downloads/mkclean.html

@JeromeMartinez

This comment has been minimized.

Show comment
Hide comment
@JeromeMartinez

JeromeMartinez Aug 5, 2017

and in that case remuxing with mkvmerge is better anyway.

I didn't think of that, right it may be enough for most people.
I am just aware of some people trying to keep the same structure as the original file when they edit / add metadata i.e. no remuxing (it changes lot of things), but I guess it is a too much specific need for being inside this ticket, and such person would open a dedicated ticket if he needs that.

JeromeMartinez commented Aug 5, 2017

and in that case remuxing with mkvmerge is better anyway.

I didn't think of that, right it may be enough for most people.
I am just aware of some people trying to keep the same structure as the original file when they edit / add metadata i.e. no remuxing (it changes lot of things), but I guess it is a too much specific need for being inside this ticket, and such person would open a dedicated ticket if he needs that.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Aug 5, 2017

Owner

Does mkclean do this kind of re-ordering?

Possibly. Summoning @robUx4 Steve, does mkclean do that?

Owner

mbunkus commented Aug 5, 2017

Does mkclean do this kind of re-ordering?

Possibly. Summoning @robUx4 Steve, does mkclean do that?

@robUx4

This comment has been minimized.

Show comment
Hide comment
@robUx4

robUx4 Aug 11, 2017

If you use --remux yes.

robUx4 commented Aug 11, 2017

If you use --remux yes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.