Embedded cue sheet support for audio files. #982

Closed
wants to merge 9 commits into
from

Projects

None yet

5 participants

@PSyton

Embedded CUE sheet loaded form tag field 'cuesheet' like foobar2000 does it.
Playback works fine but there are some strange behavior on track change. Looks like GUI doesn't update propertly.

@elupus
Team Kodi member
@PSyton

Looks like it works now. I've test it on win.

@DDDamian

Merge window is approaching - if you want this to be added please rebase per elupus's comment above

@PSyton

it improves readability, but it is not necessary. Ok, i will ad it to final code. In next commit i've refactored this code.

Base class is virtual and it derived classes we can drop out 'virtual' keyword. But we write it for better understandig.
In later commits which I've made later done massive rafactoring and added 'virtual'.

Ok. I've already fixed this d565dad

psyton added some commits Jun 3, 2012
@PSyton

Maybe i need to squash it into one commit?

@jmarshallnz
Team Kodi member

line.clear() is normally better - no insistence though.

@jmarshallnz

Please no - you've just done a stat for "" every time you create a CMusicInfoTag. The CMusicInfoTag should not need to know about the loader IMO - put this where it needs to be instead.

I can add test for "".
But if i revert this to original, I need write 5 lines of code every time where I need tag:

if (!tag.Loaded() )
{ // read the tag from a file
  auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(pItem->GetPath()));
  if (NULL != pLoader.get())
    pLoader->Load(pItem->GetPath(), tag);
}

it's not good idea too.

@jmarshallnz
Team Kodi member

No - we don't want to attempt to load the tag every time we Get() it.

But it's a GetMusicInfoTag()! - when it's called - we need the tag if it exists.
Why need we check for loading and load (if it's not done yet) every time when we call GetMusicInfoTag()?

Team Kodi member

Because most of the time you don't want to read the tag from the file. We might want to fill it from the database for example.. The small number of places where you actually want to read it from a file can be handled by reading it there and then.

I have no problem with helpers for this purpose should you feel it's needed (eg there's one in CFileItem IIRC).

Ok. I agreed.
Look at this c5d32c3
May be it's more clean and DRY.

@jmarshallnz
Team Kodi member

This should load the tag from the file only if we need to (i.e. check whether the fileItem already has the tag loaded). If it doesn't have the tag loaded, we should only read it if tag reading is enabled.

ATM you've just made a large directory of flacs load very slowly in the hope that one of them might contain a cue sheet.

Later I've added cache for scanned files.

@jmarshallnz

cosmetics

@jmarshallnz

I'd accept this only if it's well doxy'd that the constructor reads the tag from the file if a non-empty path is specified.

ok.

@jmarshallnz

whitespace at eol

@PSyton

There is no members which can't be copied by default operator=.

@PSyton

This is more simple and we can't forget to update it when change list of members.

@HeresJonny

Can this be added to the next 13.0.alpha_x Milestone?

@PSyton PSyton closed this Jun 28, 2013
@PSyton PSyton deleted the PSyton:cue_sheet_support branch Jun 28, 2013
@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Jan 17, 2014
@tru tru Unify stream string information and add (forced) to subtitles that ar…
…e forced

This brings us to parity with Plex/Web

Fixes #982
d2bf8db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment