mkv, mp4 track detection improvements

[sscobici]

I will try to create small PRs for easy review.
Dolby Vision HEVC files have more configuration atoms (side data) besides hvcC, for example dvcC
It might be useful if VideoCodecParameters.extra_data to be a map of buffers instead of a single buffer.

[pdeljanov]

Wouldn't it make sense to put this error in the _ match arm below?

[sscobici]

I just copied the pattern from above VisualSampleEntryAvc1, I think this check doesn't allow parsing known sample entry twice. To keep the same behavior decode error will need to be repeated for each known variant.

Is naming VisualSampleEntry better than VideoSampleEntry?

[pdeljanov]

This is just parsing the HEVCDecoderConfigurationRecord, let me know what you think about my comment from your MKV PR.

[sscobici]

will create separate crate to reuse the code.

[sscobici]

Hopefully these changes will not introduce regressions with audio playback.

[sscobici]

rebased, fixed symphonia-check compilation.

On a separate topic:
VideoCodecParameters need enhancements.

There is a need to store multiple codec buffers per track
pub extra_data: Option<Box<[u8]>>,

Do you want me to convert extra_data property to a dictionary where keys will be an Enum:
"HEVCDecoderConfigurationRecord",
"AVCDecoderConfigurationRecord",
"DolbyVisionConfiguration",

and values to be Box ?

I would also add Hdr enumeration to understand if this file is a SDR, HDR10, HDR10+ or DolbyVision

Do you like me to open a discussion on this topic, or I can create a PR with proposed changes and you will review.

[pdeljanov]

Hello,

It's a holiday this weekend so I haven't gotten to doing a deep review, however, I've skimmed it over and have some thoughts.

---

The MP4 changes look reasonable and are pretty low risk for regression. I think we can likely merge those as soon as I finish a more thorough review pass.

---

The changes to symphonia-check probably need some more work. I suggest we move it to a new PR.

If we are going to expand the scope of tests symphonia-check can perform, I think we should use sub-commands to select the test. For example, the existing audio test could be symphonia-check audio .... Your new test could be symphonia-check info ..., etc. I think this is better than overloading the refdec option to alter the behaviour.

I'm also not enthusiastic about relying on a format.txt. I think it'd be better if we could have a built-in default format string that could be overridden with an option to the info sub-command. This should be pretty simple since either the default format string or the override just needs to be passed to mediainfo.

---

On the topic of VideoCodecParameters, we can make extra_data a Vec for video, however, it cannot be an enum. The core crate should never create interfaces that are specific to any particular codec or format. You can however do something similar to VideoCodecId and have some well-known ones defined in the well_known sub-module. That way developers outside the project can define their own extra data types.

struct VideoExtraDataType(u32);

struct VideoExtraData {
   id: VideoExtraDataType,
   data: Box<[u8]>,
}

struct VideoCodecParameters {
  // ...
  extra_data: Vec<VideoExtraData>,
}

mod well_known {
  pub const EXTRA_DATA_TYPE_HEVC_DECODER_CONFIG_RECORD: VideoExtraDataId = VideoExtraDataId(0);
  // ...
}

Let's do this one in a separate PR as well since there will probably be some iteration on it.

[sscobici]

removed commit for check, will make separate PRs.
It seems that converting of extra_data to vector will affect some other classes: like TrackElement in mkv and probably others.

[pdeljanov]

Thanks, merged!

It seems that converting of extra_data to vector will affect some other classes: like TrackElement in mkv and probably others.

Hmm, if it's just for VideoCodecParameters, then it's all new code anyways. You don't need to really worry about breaking anything that's in production.

However, I did miss a couple things in my last message.

In the code example I gave, the value of 0 should be reserved for a "null"/"default" buffer. Atleast for audio, there wasn't a need for multiple extra data buffers, so there was no need to label it. I imagine that older video codecs could be same. In these cases the default type could be used. A second option is to not have a default type, but just make the type optional.

Option 1:

struct VideoExtraDataType(u32);

pub const EXTRA_DATA_TYPE_DEFAULT: VideoExtraDataType = VideoExtraDataType(0x0);

mod well_known {
  // ...
  pub const EXTRA_DATA_TYPE_HEVC_DECODER_CONFIG_RECORD: VideoExtraDataType = VideoExtraDataType(0x1);
  // ...
}

struct VideoExtraData {
   data_type: VideoExtraDataType,
   data: Box<[u8]>,
}

struct VideoCodecParameters {
  // ...
  extra_data: Vec<VideoExtraData>,
}

Option 2:

struct VideoExtraData {
   data_type: Option<VideoExtraDataType>, // Can be `None` if codec only need one extra data buffer.
   data: Box<[u8]>,
}