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

mp4: parse udta stream names #1281

Merged
merged 1 commit into from Jun 5, 2021

Conversation

JoelLinn
Copy link
Contributor

@JoelLinn JoelLinn commented Jun 2, 2021

  • Increases accessibility to the visually impaired by allowing multiple
    (audio) channels per language.
  • Stream names are usually (i.e. FFmpeg) saved in udta node as
    name atom
  • Configurable via vod_parse_udta_name, defaults to Off
  • Existing hdlr name parsing often just contains generic names, see
    is it possible to get audio label attribute from mp4 file in local mode? #695 for previous
    discussion

I am assuming null terminated strings inside udta->name atom. I know that some formats (.mov ?) use pascal strings. At least FFmpeg uses null termination for mp4, please comment if you think pascal strings might be encountered and need to be addressed (similar to hdlr name is parsed)

If both vod_parse_hdlr_name and vod_parse_udta_name are On, hdlr will take precedence. I consider this an implementation detail and therefore did not specify this order in the documentation/Readme.md

@kaltura-hooks
Copy link

Hi @JoelLinn,
Thank you for contributing this pull request!
Please sign the Kaltura CLA so we can review and merge your contribution.
Learn more at http://bit.ly/KalturaContrib

vod/mp4/mp4_parser.c Outdated Show resolved Hide resolved
vod/mp4/mp4_parser.c Outdated Show resolved Hide resolved
vod/mp4/mp4_parser.c Outdated Show resolved Hide resolved
vod/mp4/mp4_parser.c Outdated Show resolved Hide resolved
vod/mp4/mp4_parser.c Outdated Show resolved Hide resolved
@JoelLinn
Copy link
Contributor Author

JoelLinn commented Jun 4, 2021

Changed as requested

@erankor
Copy link
Contributor

erankor commented Jun 4, 2021

I checked ffmpeg source -
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/movenc.c#L3279

Unlike hdlr, there is no case of pascal string here, and also, it seems ffmpeg won't write any null characters on this string.
So, unless you have a sample file created with some other software that has these properties, I prefer to remove this code. Meaning - just use name & name_len as they are.

@JoelLinn
Copy link
Contributor Author

JoelLinn commented Jun 4, 2021

special string handling removed

@erankor
Copy link
Contributor

erankor commented Jun 5, 2021

One last comment, for consistency with the hdlr code, let's replace const char* name & uint64_t name_len with a single var vod_str_t name

- Increases accessibility to the visually impaired by allowing multiple
  (audio) channels per language.
- Stream names are usually (i.e. FFmpeg) saved in `udta` section as
  `name` atom
- Configurable via `vod_parse_udta_name`, defaults to `Off`
- Existing `hdlr` name parsing often just contains generic names, see
  kaltura#695 for previous
  discussion
@JoelLinn
Copy link
Contributor Author

JoelLinn commented Jun 5, 2021

👍

@erankor
Copy link
Contributor

erankor commented Jun 5, 2021

Merged, thanks!

@erankor erankor closed this Jun 5, 2021
@erankor erankor reopened this Jun 5, 2021
@erankor erankor merged commit 56a5407 into kaltura:master Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants