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

[Feature] Loading/Play external audio tracks #1337

Closed
wants to merge 5 commits into from

Conversation

ace20022
Copy link
Member

This PR adds the possibility to play external audio tracks together with video files and stacked videos.
At the moment the files have to be placed either in the same directory or in a subdirectory named "audio" or "tracks". The files can also be zipped or rared.
The recognised file extensions are read from g_settings.m_musicExtensions.

This closes ticket http://trac.xbmc.org/ticket/11446 .

@ace20022
Copy link
Member Author

@sialivi I added it here SHA: ab3b9500fc5b0118ca452c60c65cfb40371b1f79 . If you know some more please post them in the forum.

@Memphiz
Copy link
Member

Memphiz commented Aug 28, 2012

All in all its pretty hard to review. You should resquash into logical commits.

@ace20022
Copy link
Member Author

Will do so. Should I close this one and open a new one or can this be done in the same one? (I am very unversed in git/github) Sorry!

@Memphiz
Copy link
Member

Memphiz commented Aug 28, 2012

This can be done in this one. Clean it up locally and then force push it to your repo ace20022/master - this will automatically update this pull request then.

Squashing can be done be either rebasing locally (git rebase HEAD~2 -i ... then prefix the second commit with "f" - this will squash the top most 2 commits into one). Its hard to give a quick howto here - but the github help is pretty good for getting the feeling on what can be done.

@DDDamian
Copy link
Contributor

Not sure what platform you're using for dev ace20022, but if on Windows let me know - fairly easy there.

@ace20022
Copy link
Member Author

I'm afraid I'll have to redo this completely from the start, since the chances are scattered all over the commits. Sorry for the mess :(

@ace20022
Copy link
Member Author

Now I'm done with the reorganisation of the base feature set.

@DDDamian Thank you for offering your support! (My platform is indeed Windows)

@ace20022
Copy link
Member Author

@vdrfan Will do so. Since these two methods are basically copies of the external subtitle methods, your comment is also valid for them, I guess.

@mkortstiege
Copy link
Member

Yea, external subtitle/audio track makes no sense for live TV and the likes.

@ace20022
Copy link
Member Author

I noticed that the DVDFileInfo part is not as easy as I thought at first. I will add a commit soon.

@ace20022
Copy link
Member Author

I noticed that the DVDFileInfo part is not as easy as I thought at first. I will add a commit soon.

done, I altered the relevant commit itself.

@ace20022
Copy link
Member Author

ace20022 commented Sep 2, 2012

"." is not good filename parts delimiter - f.e. "The.Dark.Knight.Rises." will fail with this approach

Yes, it's not perfect. Imo a user who wants "nice" external audio tracks has to care for himself about it.

I think that language and "type" extraction might be done in scanforexternalaudio/subtitles methods - we could clean external audio/subtitles filename of base video filename easily and interpret what's remain as language and/or type

I also thought about diffing the the names. I have a sketch on my hd, but in the DVDFileInfo file. The problem there is that you can not easily get the video filename; more precisely I'm not sure what to do if there is more than one video stream. Furthermore, if the video is named "The.Dark.Knight.Rises.English" then the external track has to have the name "The.Dark.Knight.Rises.English.English".

Doing this in scanforexternalaudio/subtitles methods will be "nicer".

Will have a look at this asap.

@ace20022
Copy link
Member Author

ace20022 commented Sep 3, 2012

@pieh
I implemented the file name diffing part, but not in scanforexternal. The reason is that if we do so, we have to elevate the infos to the dvdplayer (the thumbloader part is a completely different thing, anyway).
I thought about a struct and a member vector for the dvdplayer, like

struct externalAudioTrack
{
    CDVDInputStream* m_extInputStream;
    CDVDDemux* m_extDemuxer;
    CStdString lang;
    CStdString type;
    CStdString code;
}

But then I have to rewrite almost everything. If this is a stopper for this PR I will do it after the review.

The reason why I haven't added a commit is that I found an access violation.
I use(d) CLangCodeExpander::ConvertToThreeCharCode(CStdString& strThreeCharCode, const CStdString& strCharCode, bool localeHack /*= false*/)
to get a language code from a language name, e.g., English. The point is that this method sets the global locale of the system. I really have no clue why the method is named like this!? The crash occurs when xbmc starts and a movie with tagged ext. audio files is in the recently added list, so its maybe a thread thing.

I think the easiest way would be to parse lang. codes directly, i.e., users have to name their files accordingly like "The Matrix.eng-Commentary.ac3". What do you think?

@pieh
Copy link
Contributor

pieh commented Sep 3, 2012

@ace20022
In paralel to your work i'm doing something similiar for external subtitles

I added method to convert language string (being 2char code "en", 3char code "eng" or full language name "english") to 2char code: pieh@4b9cc5d

And helper method to extract language and name from filename: pieh@5789502 (i dropped idea of doing this as part of CUtil::ScanForExternalXX as we can manually point to subtitles or load subitltes from upnp resource and lang extraction would be bypassed)

What I do there is specify few regexpes to match lang and name and try to pick up best from all matches. Fact that we know if string that is recognized as lang part is actual part (testing if we can convert it to 2char code) makes this pretty accurate. I think that I would also add either white list and/or black list for stream names. Regexpes and white/black list name entries would be adjustable via advancedsettings.xml

@ace20022
Copy link
Member Author

ace20022 commented Sep 3, 2012

@pieh
Really great! Way better than my stuff! Is this final and is it possible to merge your commits in my branch?

@pieh
Copy link
Contributor

pieh commented Sep 3, 2012

This is still work in progress. For now I would just drop language extraction for external audio and would hook it up when we're both ready.

@ace20022
Copy link
Member Author

ace20022 commented Sep 4, 2012

As @pieh suggested, I dropped the lang/type extraction and rebased on this occasion.
I will reimplement after/if PR #1365 got merged.

@ace20022
Copy link
Member Author

Just rebased. Has anyone from the team the time and interest to review this? I guess @elupus is the main dev for this part of the system, right?

@DDDamian
Copy link
Contributor

DDDamian commented Oct 2, 2012

Bump for review - @pieh, @elupus?

@ace20022
Copy link
Member Author

fixed some bugs

@ace20022
Copy link
Member Author

Revised version, included simplifications.

…dapt all relevant places where m_pDemuxer is used; in particular ReadPacket(...) and add ReadExternalAudioPacket(...)
(External) audio tracks don't have chapters, so we have to use SeekTime().
@ace20022
Copy link
Member Author

@elupus does this pr has any chance at all to be merged before hell freezes? :-) If not I would like to close it.

@ace20022
Copy link
Member Author

ace20022 commented Apr 3, 2013

Not even 2 bytes and a few seconds left?
I know the team follows the rule to simply ignore unwanted features, but I don't think that this is right. Over 1/3 of the prs are older then 6 month and 191 opened at the moment are too much imho.

@ace20022 ace20022 closed this Apr 3, 2013
@ghost
Copy link

ghost commented Apr 3, 2013

please, if you reopen i'll handle it.

@ace20022
Copy link
Member Author

ace20022 commented Apr 4, 2013

@cptspiff Thanks for the response! I'm afraid I can't reopen it because I changed the layout of my repo.
I will open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants