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.

Sign up for GitHub

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

Jump to bottom

read metadata from udta #77

Merged
merged 1 commit into from
Jul 21, 2022
Merged

read metadata from udta #77

merged 1 commit into from
Jul 21, 2022

Conversation

ririsoft
Copy link
Contributor

Hello,

First I would like to thank you for your work on 'mp4'. I built several implementations of the mp4 standard in Go and Rust for private use that I cannot share, but I would like to join forces to your open initiative and start to contribute.

My main focus for now is metadata parsing : title, release year, description, cover art and more.

I am starting with a draft pull request to gather early feedback from you : is the direction I am pushing the right approach for metadata handling in mp4-rust ?

This introduces the 'Metadata' trait to enable access to common video metadata such title, year, cover art and more.
Reading 'title' and 'year' metadata is implemented as a proof of concept.

Using a trait allow's for using deep internal mp4 atom structure and shallow copy (such lossy utf8 conversion) while keeping the access to metadata user friendly at top level Mp4Reader.

Once agreed on the read part and Metadata API I will implement the write part in the pull request before you merge.

Last : I added a new sample based on open source video Big Buck Bunny with some encoded metadata.

Your comments and request for changes are more than welcome !
Cheers.

@ririsoft ririsoft mentioned this pull request Jun 16, 2022
Open
@alfg
Copy link
Owner

alfg commented Jun 20, 2022

Hey @ririsoft, thanks for taking interest in the project. I will take a look soon, thank you!

alfg
alfg reviewed Jul 8, 2022
View reviewed changes
src/types.rs
Binary = 0x000000,
Text = 0x000001,
Image = 0x00000D,
TempoCpil = 0x000015,
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, what type is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review !
Those are the type used to format data in an ilst element. They are pretty obvious but TempoCpilwhich I have no idea what it is about. I took it from here: https://xhelmboyx.tripod.com/formats/mp4-layout.txt.
Binary is raw vector of u8, Imageis the same but you know it is an image, Textshould be valid utf_8 data.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I was referring to the TempoCpil type, but thanks for the clarification! Looks like it's referring to tmpo/cpil flag = 0x000015.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no clue of what it is for and left it for sake of completeness.

@alfg
Copy link
Owner

alfg commented Jul 8, 2022

Thanks for the PR. I looked over and I like the approach for the metadata handling. I just had one comment above. Otherwise, feel free to continue.

Thanks again!

@ririsoft
Copy link
Contributor Author

ririsoft commented Jul 9, 2022

Thanks !
I will update the PR with more metadata and remove the draft status in a few days.

@ririsoft ririsoft marked this pull request as ready for review July 10, 2022 09:18
@ririsoft
Copy link
Contributor Author

Ok then I rebased onto master and added two more metadata : poster, description.

There are more metadata, but none that I am using currently. Do not hesitate to "@" me in an issue if some users are requesting for a particular metadata not implemented yet.

Also there are other form of metadata, using XML, that can be embedded. I have almost never encountered such. We will see if an issue on that come one day.

Thanks again for your time and review.

@alfg
Copy link
Owner

alfg commented Jul 11, 2022

Sounds good. I'll take a look soon and merge if all is good. Thanks again for your contribution!

@alfg
Copy link
Owner

alfg commented Jul 13, 2022

Hi @ririsoft, I tried a few of the examples using some video files I usually test, but getting a constant failed to fill whole buffer.

cargo run --example mp4dump tears-of-steel-720p.mp4
    Finished dev [unoptimized + debuginfo] target(s) in 4.02s
     Running `target\debug\examples\mp4dump.exe qt.mp4`
failed to fill whole buffer

Same results for other examples as well, including mp4info, mp4sample and mp4dump.

@ririsoft
Copy link
Contributor Author

I can see the issue on Linux not on Mac. I am investigating further and will submit a fix asap.

@ririsoft
read metadata from udta
f6bb8f2 
This introduces the 'Metadata' trait to enable access
to common video metadata such
title, year, cover art and more.

Reading 'title', 'description', 'poster' and 'year'
metadata is implemented here.
@ririsoft
Copy link
Contributor Author

Problem identified and should be fixed : a let start = box_start(reader)?; was set after reading the box version and thus loosing 4 bytes on the way. Nothing to do with Linux or Mac but rather the video you throw it up. Please double check with your own samples.

@alfg
Copy link
Owner

alfg commented Jul 20, 2022

Thanks for the fix. Will check this out soon.

@alfg alfg merged commit ace2799 into alfg:master Jul 21, 2022
@alfg
Copy link
Owner

alfg commented Jul 21, 2022

All good. Thanks again!

jprochazk pushed a commit to jprochazk/mp4 that referenced this pull request Sep 18, 2024
@ririsoft
read metadata from udta (alfg#77)
65ed252 
This introduces the 'Metadata' trait to enable access
to common video metadata such
title, year, cover art and more.

Reading 'title', 'description', 'poster' and 'year'
metadata is implemented here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alfg alfg alfg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ririsoft @alfg