search

bbc / bmx

Library and utilities to read and write broadcasting media files. Primarily supports the MXF file format
BSD 3-Clause "New" or "Revised" License
57 stars 16 forks source link

Sample rate restriction via MXFDescriptorHelper::SetSampleRate #70

Closed irieger closed 2 months ago

irieger commented 2 months ago

Hey,

I'd like to understand what the reasoning is to restrict the sample rate very strictly in the MXFDescriptorHelper::SetSampleRate method?

I was experimenting with some different frame rates - while maybe not as common - there is use cases using for example 48fps (HFR cinema) or experimenting with "real" HFR at 100/120fps. Also is there something specifying that a MXF can't hold arbitrary frame sample rates? While I don't see a specific use to use something like 42fps or whatever, I'd be curious why the container is preventing it. Also for like time lapse or slow motion data to me it would be more logical to define the actual fps to represent the content semantically the way it was captured and then in editing adapt to the required project rate.

Did a quick test by commenting the following part out and creating a clip at an arbitrary frame rate (just hit two numbers on the keyboard and landed on 27fps). Was no problem to get Resolve to read the file with the "correct" fps emerging from that and reassigning the needed project rate to it is also straight forward.

    BMX_CHECK(sample_rate == FRAME_RATE_23976 ||
              sample_rate == FRAME_RATE_24 ||
              sample_rate == FRAME_RATE_25 ||
              sample_rate == FRAME_RATE_2997 ||
              sample_rate == FRAME_RATE_30 ||
              sample_rate == FRAME_RATE_50 ||
              sample_rate == FRAME_RATE_5994 ||
              sample_rate == FRAME_RATE_60 ||
              (IsSound() && sample_rate == SAMPLING_RATE_48K));

And should the standard discourage such frame rates, would it maybe still be acceptable to have a parameter or setting to allow skipping this check? I thought about raising a PR but wanted to get some more background before. My idea would be to do one of the following, maybe with 1. being the more straight forward approach:

  1. Add a second, optional parameter to SetSampleRate landing at something like: virtual void SetSampleRate(mxfRational sample_rate, bool skip_sample_rate_check = false);. This could just be the first point in the chain of ||s to lazy evaluate to true if requested.

  2. Introduce a seperate method to the class, something like MXFDescriptorHelper::ApplyStrictSampleRateCheck.

P.S. Btw. thanks for the fix interaction on my two PRs. It is really a warming experience to have quick turnaround in PRs and makes it so much fun to contribute. Got to my main goal of being able to read and write MXF clips by looking how mxf2raw and raw2bmx work.

philipnbbc commented 2 months ago

The sample rate check in MXFDescriptorHelper, which covers all codecs, probably doesn't need to be there and is better handled in the precise places where rate support is limited. The list is also biased towards frame/sampling rates used in broadcaster workflows.

If you search for "FRAMERATE" you'll see various places where the edit/sample rate is used to determine what type of essence format it is and therefore what label should be used for example. Conversely, some essence format specifications explicitly list the frames rates and associated properties and labels.

I think the solution is to remove the check completely.

The current behaviour when given an "unsupported" sample rate is to throw an exception. Removing the check will either produce a valid file, an invalid / broken file or a check will fail elsewhere. The invalid / broken file is only a minor issue because typical broadcast workflows wouldn't be effected. It could be fixed in future by either changing the code to produce valid files or by adding a missing check where it is needed. Similarly for the fail later cases if they didn't need to fail.