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

ADM (Audio Definition Model) and Dolby Metadata in WAV/RIFF #991

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

JohnnyMarnell
Copy link

@JohnnyMarnell JohnnyMarnell commented Aug 16, 2024

<chna> and <axml> chunks of Audio Definition Model:

Dolby Metadata <dbmd> chunk, e.g. Atmos, AC3, Dolby Digital [Plus]:

Notes:

  • Had to modify the riff decode fn handlers to receive the chunk size, some need it
  • Might be missing some fq + Go idiomatics, happy to correct!

@wader
Copy link
Owner

wader commented Aug 16, 2024

Hey! nice! will review asap, today or during the weekend at latest

@wader
Copy link
Owner

wader commented Aug 16, 2024

To extract a field as binary you can do this:

# stdout is a tty so does fq's own hexdump
$ fq '.chunks[0] | tobytes' -o line_bytes=10 format/riff/testdata/bext.wav
     │00 01 02 03 04 05 06 07 08 09│0123456789│
0x000│62 65 78 74 5a 02 00 00 00 00│bextZ.....│.: raw bits 0x0-0x262 (610)
0x00a│00 00 00 00 00 00 00 00 00 00│..........│
0x014│00 00 00 00 00 00 00 00 00 00│..........│
*    │until 0x261.7 (end) (610)    │

# but if stdout is pipe it will output raw bytes
$ fq '.chunks[0] | tobytes' format/riff/testdata/bext.wav | hexdump -C
00000000  62 65 78 74 5a 02 00 00  00 00 00 00 00 00 00 00  |bextZ...........|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000100  00 00 00 00 00 00 00 00  52 45 41 50 45 52 00 00  |........REAPER..|
00000110  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000140  00 00 00 00 00 00 00 00  32 30 30 31 2d 30 32 2d  |........2001-02-|
00000150  30 33 30 34 2d 30 35 2d  30 36 00 00 00 00 00 00  |0304-05-06......|
00000160  00 00 01 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000170  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000260  00 00                                             |..|
00000262

To concat/stitich things you can do something like this, sorry not the greatest example:

$ fq '.chunks[0] | [(.originator | tobytes[2:4]), "hello", 0xff, .version] | tobytes ' format/riff/testdata/bext.wav | hexdump -C
00000000  41 50 68 65 6c 6c 6f ff  01 00                    |APhello...|
0000000a

@wader
Copy link
Owner

wader commented Aug 16, 2024

Any idea what the license is for the official EBU ADM test files? ok to modify and distribute? sometimes i might be enough to strip out the audio etc somehow and keep just the parts fq will decode

I know that some linux distributions like debian are not happy with distribution the source package if it contains anything with restrictive license.

@wader
Copy link
Owner

wader commented Aug 16, 2024

Found this https://tech.ebu.ch/files/live/sites/tech/files/shared/testmaterial/ebu_test_sequences_policy_prices_190315.pdf not sure if it covert the adm files but sadly sounds very restrictive :( hmm whyyyy


### Authors
- [@johnnymarnell](https://johnnymarnell.github.io), original author

Copy link
Owner

@wader wader Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to a doly_metadata.md file in format/tiff, formats.md is autogenerated via make doc. Then also add something like this https://github.com/wader/fq/blob/master/format/msgpack/msgpack.go#L16-L29 and the documentation will also ge available in the cli fq -h dolby_metadata and help(dolby_metadata)

Same for adm above

"github.com/wader/fq/pkg/scalar"
)

func dbmdDecode(d *decode.D, size int64) any {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hmm maybe you want to register this and adm as proper own formats? and then use them as subformat in the wav decoder? then they could be reused in other containers (is it used in mp4?) and be used with -d if that makes sense?

id, size := headFn(d, path)

d.FramedFn(size*8, func(d *decode.D) {
hasChildren, data := chunkFn(d, id, path)
hasChildren, data := chunkFn(d, id, path, size)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses FramedFn so i think chunkFn will be decoding in a limited/framed bit range so i think you can use BitsLeft/Len etc to get how much is left

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have been doing it wrong, but that's what I tried initially, hope to get to revisit soon!

Copy link
Owner

@wader wader Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okok yeah in this case i think relying on framing should work and i think it usually ends up with nicer code, let's try that first at least

@wader
Copy link
Owner

wader commented Aug 20, 2024

Hey! spent some time learning about adm, atoms etc and made things a bit more fq idiomatic here https://github.com/wader/fq/tree/jm/adm

@JohnnyMarnell
Copy link
Author

Found this https://tech.ebu.ch/files/live/sites/tech/files/shared/testmaterial/ebu_test_sequences_policy_prices_190315.pdf not sure if it covert the adm files but sadly sounds very restrictive :( hmm whyyyy

I was able to make a quite small ADM BWF, hope to get it in + testes soon!

@wader
Copy link
Owner

wader commented Aug 20, 2024

So feel free to push my changes to this branch, i might continue on it later in the week

@wader
Copy link
Owner

wader commented Aug 21, 2024

Pushed some more stuff, now dolby_metadata is a proper format, not sure if adm should be one also hmm?

Maybe could be nice to squash all the commits to get a more clean picture

@wader
Copy link
Owner

wader commented Aug 21, 2024

still confused about trim config stuff and program_info, do you have any spec (verison 0.0.0.7?) or code that parses it. the dolby code seems to just skip it? (why?? 😄 )

return
}

segmentSize := d.FieldU16("metadata_segment_size")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: spec says "Valid range: 0 - 65535 (0 is interpreted as 65535 byte)." but it seems mediainfo does not care about 0 but do truncate the size

@JohnnyMarnell
Copy link
Author

Pushed some more stuff, now dolby_metadata is a proper format, not sure if adm should be one also hmm?
Maybe could be nice to squash all the commits to get a more clean picture

Integrated your changes (finally) (thanks so much!), going to look into the formats / docs, since proprietary Dolby is separate. And will get out my git manual and attempt commits clean up 🥴

@JohnnyMarnell
Copy link
Author

TIL MediaInfo, thank so much for the reference...

@wader
Copy link
Owner

wader commented Oct 29, 2024

Pushed some more stuff, now dolby_metadata is a proper format, not sure if adm should be one also hmm?
Maybe could be nice to squash all the commits to get a more clean picture

Integrated your changes (finally) (thanks so much!), going to look into the formats / docs, since proprietary Dolby is separate. And will get out my git manual and attempt commits clean up 🥴

👍 if you do make doc you will see how it works, both README.md and doc/formats.md will be updated automatically based on registered formats and doumentation.

It would be great if you could more or less squash all into one or few commits, preferably without merge commits.

@wader
Copy link
Owner

wader commented Oct 29, 2024

TIL MediaInfo, thank so much for the reference...

It's nice! similar to fq but less queryable and more about interpreting/validating things. Meet the main developer of it some years ago at a "no time to wait" conference, both there to present and yap about our tools :)

doc/dev.md Outdated
- If possible, add one or more pairs of example input file and expected CLI output, with naming like:
- `./format/<format_name>/testdata/<name>.<ext>`, e.g. [`./format/mp4/testdata/aac.mp4`](../format/mp4/testdata/aac.mp4)
- and `./format/<format_name>/testdata/<name>.fqtest`, e.g. [`./format/mp4/testdata/aac.fqtest`](../format/mp4/testdata/aac.fqtest)
- The latter contents should be `$ go run . dv <file_path>` or `$ go run . 'dv,torepr' <file_path>` if there is `torepr` support.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be $ fq .... The .fqtest files looks very much like shell scripts but they are more like "transcripts" that the fq test system deserialize/serialize and execute using the go test system, so no there is no external processes executed per test etc.

### References
- https://adm.ebu.io/background/what_is_the_adm.html
- https://tech.ebu.ch/publications/tech3285s7
- https://tech.ebu.ch/publications/tech3285s5
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move to a wav.md file? only "real" formats can have .md-files, they get added via the RegisterFS stuff

@@ -54,7 +54,7 @@ func aiffDecode(d *decode.D) any {
}
return id, size
},
func(d *decode.D, id string, path path) (bool, any) {
func(d *decode.D, id string, path path, size int64) (bool, any) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size not needed anymore?

@@ -238,7 +238,7 @@ func aviDecodeEx(d *decode.D, ai format.AVI_In, extendedChunk bool) {
size := d.FieldU32("size")
return id, int64(size)
},
func(d *decode.D, id string, path path) (bool, any) {
func(d *decode.D, id string, path path, size int64) (bool, any) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size not needed anymore?

@@ -0,0 +1,192 @@
|00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f|0123456789abcdef|.{}: ./format/riff/testdata/dolby_metadata.wav (wav) 0x0-0x2aec (10988)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a starting $ fq dv dolby_metadata.wav line etc here. Then you can use -updatewhen you run the tests to automatically update with expected output

d.FieldValueStr("config_name", trimConfigName[uint64(i)])

// TODO: this is null separted list of def strings?
d.FieldUTF8("raw", 14)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use same name as mediainfo? "reserved"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do for auto case, looks like there's more to do for manual, thanks again for the idea!
https://github.com/MediaArea/MediaInfoLib/blob/master/Source/MediaInfo/Audio/File_DolbyAudioMetadata.cpp#L381

@JohnnyMarnell
Copy link
Author

@wader One of last steps is to remove dolby.go, currently in there because of some comments I need to migrate, but mainly because the "dolby supplemental" metadata segment appears to still work and parse there, but I'm not seeing it in the new dolby_metadata.go code

@wader
Copy link
Owner

wader commented Nov 1, 2024

@wader One of last steps is to remove dolby.go, currently in there because of some comments I need to migrate, but mainly because the "dolby supplemental" metadata segment appears to still work and parse there, but I'm not seeing it in the new dolby_metadata.go code

Hmm stange, if i remove dolby.go it seems to build fine for me. But i don't see any supplement metadata_segment for the tests?

@JohnnyMarnell
Copy link
Author

@wader Sorry, that wasn't clear, tests don't currently reflect it. If you run and compare this command's output as currently:

go run . -d wav '.chunks[] | select(.id | IN("dbmd")) | tovalue' ./format/riff/testdata/dolby_metadata.wav

vs after commenting and switching d.Format() for old_dbmdDecode(d) here: https://github.com/JohnnyMarnell/fq/blob/jm/adm/format/riff/wav.go#L188-L190 , you can see the dolby_atmos_supplemental isn't getting parsed currently. Not spotting why tho, I'm assuming it's something dumb I'm doing.

@wader
Copy link
Owner

wader commented Nov 3, 2024

@wader Sorry, that wasn't clear, tests don't currently reflect it. If you run and compare this command's output as currently:

go run . -d wav '.chunks[] | select(.id | IN("dbmd")) | tovalue' ./format/riff/testdata/dolby_metadata.wav

vs after commenting and switching d.Format() for old_dbmdDecode(d) here: https://github.com/JohnnyMarnell/fq/blob/jm/adm/format/riff/wav.go#L188-L190 , you can see the dolby_atmos_supplemental isn't getting parsed currently. Not spotting why tho, I'm assuming it's something dumb I'm doing.

Hmm try this, it seems to make things work. Probably one or more segment decoders don't read their full segment size. Using d.FramedFn it will make sure to limit and seek correctly.

diff --git a/format/riff/dolby_metadata.go b/format/riff/dolby_metadata.go
index 56a52113..50a9a717 100644
--- a/format/riff/dolby_metadata.go
+++ b/format/riff/dolby_metadata.go
@@ -51,23 +51,25 @@ func dbmdDecode(d *decode.D) any {
 
 				segmentSize := d.FieldU16("size")
 
-				switch segmentID {
-				case metadataSegmentTypeDolbyE:
-					parseDolbyE(d)
-				case metadataSegmentTypeDolbyDigital:
-					parseDolbyDigital(d)
-				case metadataSegmentTypeDolbyDigitalPlus:
-					parseDolbyDigitalPlus(d)
-				case metadataSegmentTypeAudioInfo:
-					parseAudioInfo(d)
-				case metadataSegmentTypeDolbyAtmos:
-					parseDolbyAtmos(d)
-				case metadataSegmentTypeDolbyAtmosSupplemental:
-					parseDolbyAtmosSupplemental(d)
-				default:
-					d.FieldRawLen("unknown", int64(segmentSize*8))
-				}
-
+				d.FramedFn(int64(segmentSize)*8, func(d *decode.D) {
+
+					switch segmentID {
+					case metadataSegmentTypeDolbyE:
+						parseDolbyE(d)
+					case metadataSegmentTypeDolbyDigital:
+						parseDolbyDigital(d)
+					case metadataSegmentTypeDolbyDigitalPlus:
+						parseDolbyDigitalPlus(d)
+					case metadataSegmentTypeAudioInfo:
+						parseAudioInfo(d)
+					case metadataSegmentTypeDolbyAtmos:
+						parseDolbyAtmos(d)
+					case metadataSegmentTypeDolbyAtmosSupplemental:
+						parseDolbyAtmosSupplemental(d)
+					default:
+						d.FieldRawLen("unknown", d.BitsLeft())
+					}
+				})
 				// TODO: use this to validate parsing
 				d.FieldU8("checksum", scalar.UintHex)
 			})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants