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

pbcore_2 - maps General_Format to instantiationStandard https://githu… #787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kieranjol
Copy link
Contributor

…b.com//issues/786

Hey, I tried building mediainfolib and mediainfo on osx and I couldn't see the changes appear - possibly i'm doing something wrong with the build or these changes are not enough.

Anyhow, does this look like it could work?
#786

@JeromeMartinez
Copy link
Member

Tested, and I have the expected output:

	<instantiationDigital>audio/vnd.wave</instantiationDigital>
	<instantiationStandard>Wave</instantiationStandard>
	<instantiationLocation>blabla.wav</instantiationLocation>

Now wondering what to do with the old "Format" part, duplicate content but if we remove the old "format" part it may break some automated parsings.

On hold until Dave reviews it.

@kieranjol
Copy link
Contributor Author

Hey, I finally got it to build (make clean is my friend) - and I also get the expected output. I think Standard is in the correct order of PBCore elements as well. I was thinking about the old "Format" part as well ,but I'm thinking it could break old parsings. I think there is no harm in leaving it in as an annotation.

@kieranjol
Copy link
Contributor Author

If it helps, here's three outputs with my patch:
https://gist.github.com/kieranjol/6572e72159d1c95a05c40b30e3783b9a

Matroska, QuickTime, Mpeg-4

        <instantiationDigital>video/x-matroska</instantiationDigital>
        <instantiationStandard>Matroska</instantiationStandard>
	<instantiationDigital>video/quicktime</instantiationDigital>
	<instantiationStandard>QuickTime</instantiationStandard>
	<instantiationDigital>video/mp4</instantiationDigital>
	<instantiationStandard>MPEG-4</instantiationStandard>

@JeromeMartinez
Copy link
Member

@kieranjol maybe flagging the duplicate content in the source code, e.g. a comment "//TODO remove this line at next version bump" in the corresponding source code line? So we can track duplicate content and clean up the output when we can.

@kieranjol
Copy link
Contributor Author

Sounds good to me - I'm just having difficulty finding the specific line of code that is pulling that Format annotation... I can see the instantiationAnnotations section in the code, but I'm not seeing that specific element - I see Format/URL and Format_Commercial..

@JeromeMartinez
Copy link
Member

A new public release is expected next week. @dericed something you want to change in this PR?

@kieranjol
Copy link
Contributor Author

Ping

@kieranjol
Copy link
Contributor Author

@dericed ping.. just wondering if this looks OK, can it be merged or do you want to wait until you've done some refactoring as mentioned in #786?

@dericed
Copy link
Collaborator

dericed commented Mar 31, 2018

I have some notes on this field from a meeting with WGBH but I’m afk today.

@kieranjol
Copy link
Contributor Author

Ah cool, I look forward to hearing more. Enjoy the AFK, i'm going to do the same.

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.

3 participants