Unify the assignment and usage of the name and language of streams #2162

Merged
16 commits merged into from Apr 2, 2013

Projects

None yet

5 participants

@ace20022
Member
ace20022 commented Feb 2, 2013

The first few commits improve/correct the assignment of dvd streams' name and language.
Next the get*Language methods now return a three char code if possible.
This code is used to show the full language name in the gui.

@elupus
Member
elupus commented Feb 2, 2013

Instead of adding yet another function to IPlayer could you add
getstreaminfo(int type, int index, struct&info) instead so we can remove
all the other.
On Feb 2, 2013 2:30 PM, "Andreas Zelend" notifications@github.com wrote:

At the moment "unnamed" is shown if a stream has no name set, that is the
normal case for subtitles. Therefore I would suggest to remove it.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2162#issuecomment-13029774.

@ace20022
Member
ace20022 commented Feb 2, 2013

I'm not 100% sure what you mean since I haven't added any function to the player. Do you want to save method calls? If this is the goal I can add a commit, but I doubt that another lengthy method improves comprehensibility.

@elupus
Member
elupus commented Feb 2, 2013

Sorry, missread the diff. Will have to look at it in some more detail. But
looks good initially.

@ace20022
Member
ace20022 commented Feb 4, 2013

@elupus ideally #2128 should go in first, because it fixes a bug in get_audio_info which I would only be too happy to use.

@ace20022
Member
ace20022 commented Feb 6, 2013

Now the language of any selection stream should be a three char code. These are used for the language priority evaluations.

@elupus elupus commented on the diff Feb 7, 2013
...dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
- switch( audio_attributes.lang_extension )
- {
- case DVD_AUDIO_LANG_EXT_VisuallyImpaired:
- strLanguage+= " (Visually Impaired)";
- break;
- case DVD_AUDIO_LANG_EXT_DirectorsComments1:
- strLanguage+= " (Directors Comments)";
- break;
- case DVD_AUDIO_LANG_EXT_DirectorsComments2:
- strLanguage+= " (Directors Comments 2)";
- break;
- case DVD_AUDIO_LANG_EXT_NotSpecified:
- case DVD_AUDIO_LANG_EXT_NormalCaptions:
- default:
- break;
- }
@elupus
elupus Feb 7, 2013 Member

Why was this removed? This is the closest we get to an actual name.

@ace20022
ace20022 Feb 8, 2013 Member

The main problem is that with the current version of dvdnav the only thing that get_audio_info returns correctly is the language code, at least with any dvd I tested. The second thing was that this code fragment belonged to GetAudioStreamLanguage, I therefore removed it and then simply forgot to re-add it to GetAudioStreamName or rather search for an alternative to get this info.

I will adapt these commits after the new version of dvdnav got merged.

@da-anda
da-anda Feb 8, 2013 Member

can we remove the hardcoded labels and make them translateable instead when refactoring this?

@ace20022
ace20022 Feb 8, 2013 Member

Sure, can do so!

@ace20022
Member
ace20022 commented Feb 9, 2013

I'm working on an update, but found some other issues, e.g. audio_attributes.lang_extension is wrong, the info is store in audio_attributes.code_extension. Maybe I can push something tomorrow; when is the merge window closed?

@MartijnKaijser
Member

At the end of tomorrow

@ace20022
Member

Some issues that I've noticed and don't know how to handle correctly, so comments are highly appreciated!
Moved here: https://gist.github.com/ace20022/c9da53faefebb3d76ef5

@ace20022
Member

Had a look at blurays in the meantime, no special handling is needed for them. Btw this pr should fix the auto language selection since this also uses 3 char codes.

@ace20022
Member

@elupus Could you please have a look at this pr. It is not only a gui gimmick, at least imo, it also fixes the subtitle auto-selection and fixes, or improves, json-rpc usage, e.g., remotes w.r.t. language representation. At the moment you get the full language names, sometimes even localised and in the wrong field:
For example: https://gist.github.com/ace20022/4a5207870ff4302b9ca0

@ace20022
Member
ace20022 commented Mar 4, 2013

I have revised and extended this pr after 87047e9. It's quite large now.
There are some issues regarding libdvd that I've noticed. I opened a gist for discussion, so no mail spam will be generated: https://gist.github.com/ace20022/c9da53faefebb3d76ef5.

@davilla Could you please check the amlplayer commits? Especially 3ee15dac4af70c068774f8f01d644165a6e01ff4.

Hopefully I haven't miss anything.

@davilla
Contributor
davilla commented Mar 4, 2013

nm :) I see the previous commit that does the expansion at the source. looks fine.

@ace20022
Member
ace20022 commented Mar 5, 2013

@davilla thanks, that was really quick ;)

I see the previous commit that does the expansion at the source.

Not exactly, that's just a conversion to 3 chars if necessary. The only place where the full name is required is the gui, imo.

@davilla
Contributor
davilla commented Mar 5, 2013

do we do the convert to a full name over and over in the GUI ? if so, CPU sucker :)

@ace20022
Member
ace20022 commented Mar 5, 2013

Only for presentation in the audio/subtitle settings dialog; for subs this is done already (in the current implementation)

@ace20022
Member
ace20022 commented Mar 8, 2013

@elupus If you find some time, could you please have look? Especially at the gist thing. I'm afraid this month's deadline is coming closer ;)

@elupus
Member
elupus commented Mar 10, 2013

Sorry, out skiing. Won't be able to look until l mid week.

@ace20022
Member

No problem, have fun :)

@elupus elupus commented on an outdated diff Mar 17, 2013
...s/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.h
@@ -44,6 +44,19 @@
struct dvdnav_s;
+struct DVDStreamInfo
@elupus
elupus Mar 17, 2013 Member

DVDNavStreamInfo or make it a member of the navigator class.

@elupus
Member
elupus commented Mar 17, 2013

Fix above, rebase and then you can assign it to next merge window.

@ace20022
Member

@elupus will do soon. Any thoughts about this: https://gist.github.com/ace20022/c9da53faefebb3d76ef5 ?
I can't assign it to a merge window (not a team member), can I :) ?

ace20022 added some commits Mar 4, 2013
@ace20022 ace20022 [DVDInputStreamNavigator/Players] Use a struct and dvdnav_get_audio_i…
…nfo(...) to query all required audio stream info at once.

[Fixes]
- Fix stream type, e.g. "(Directors Comments)", recognition. It is stored in code_extension not in lang_extension.

- Properly set the number of channels of a dvd audio stream.

- Set the stream language of dvd audio tracks properly/consistent with other audio streams, i.e. set it to a three char code.

- Set the stream name of dvd audio tracks properly/consistent with other audio streams.
63bb4a6
@ace20022 ace20022 [DVDInputStreamNavigator/Players] Use a struct and dvdnav_get_stitle_…
…info(...) to query all required subtitle stream info at once.

[Fixes]
- Set the stream language of dvd subtitles properly/consistent with other streams, i.e. set it to a three char code.

- Set the stream name of dvd subtitle tracks properly/consistent with other streams; in particular don't use the language as the name.
d8b2ca8
@ace20022 ace20022 Make the type, e.g. "(Forced)", of a stream translatable in its name. d3722b9
@ace20022 ace20022 [Players] Don't assign "unknown" to the audio/subtitle stream name, l…
…eave it empty instead.
e7dfb28
@ace20022 ace20022 [DVDDemuxer] Don't set the language as the stream name. d54456c
@ace20022 ace20022 [AMLPlayer] Set the name of a stream properly/consistent with the oth…
…er players, i.e., codec name (if possible) + channels.

Don't assign "unknown" to the audio/subtitle stream name, leave it empty instead.
25b1623
@ace20022 ace20022 [Players] Convert the language code of a selection stream to three ch…
…ars if necessary.
bee3993
@ace20022 ace20022 [AMLPlayer] Convert the language code of a stream to three chars if n…
…ecessary.
d65cd9c
@ace20022 ace20022 [AMLPlayer] Always set the name and language fields of a subtitle/aud…
…io stream in the getInfo methods.
e71cb79
@ace20022 ace20022 [GUI Audio] Adapt the presentation of audio and subtitle items. The l…
…abels are built as follows:

"[language|unknown] - name" or, if name is empty, "[language/unknown]"
1d8cf16
@ace20022 ace20022 [Players] Set the language code instead of the full name in the GetSu…
…btitleStreamInfo methods.
089ba3a
@ace20022 ace20022 [DVDDemux] Add "Dolby TrueHD" and 7.1 recognition to GetStreamType. A…
…dditionally adapt the default channel string to "channel count-chs"
33b0427
@ace20022 ace20022 [Player] Return the language code of a stream, or its name if the lan…
…guage is not specified.
93e562f
@ace20022
Member

@elupus done.

@ace20022
Member

@elupus I replied here: https://gist.github.com/ace20022/c9da53faefebb3d76ef5 (no notifications triggered from gist...)

@ace20022
Member

@elupus

OK, checked it with an hexeditor in combination with ifoedit and the newer pgcedit. The correct enum is/should be:

/* The audio format */
typedef enum {
  DVD_AUDIO_FORMAT_AC3        = 0,
  DVD_AUDIO_FORMAT_UNKNOWN_1  = 1,
  DVD_AUDIO_FORMAT_MPEG       = 2,
  DVD_AUDIO_FORMAT_MPEG2_ext  = 3,
  DVD_AUDIO_FORMAT_LPCM       = 4,
  DVD_AUDIO_FORMAT_UNKNOWN_5  = 5,
  DVD_AUDIO_FORMAT_DTS        = 6,
  DVD_AUDIO_FORMAT_SDDS       = 7
} DVDAudioFormat_t;

the include comes from xbmc\cores\dvdplayer\DVDInputStreams\dvdnav\dvd_types.h . Is it sufficient to only change it there? The other files are lib\libdvd\includes\dvdnav\dvd_types.h and lib\libdvd\libdvdnav\src\dvdnav\dvd_types.h

@elupus
Member
elupus commented Mar 20, 2013

We should change all places and send a patch upstream.

@ace20022
Member

@elupus changed it. Which upstream is the correct one, since videolan forked the project?

@elupus
Member
elupus commented Mar 20, 2013

I'd say both :-)

@ace20022
Member

@elupus I proposed it here: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2013-March/thread.html.
Videolan.org has no mailing list for libdvdnav...

@ace20022
Member
ace20022 commented Apr 2, 2013

@elupus Can this one go in this merge window?

@elupus
Member
elupus commented Apr 2, 2013

Yes

@ghost ghost merged commit c3c7fd3 into xbmc:master Apr 2, 2013
@ace20022 ace20022 deleted the ace20022:unify_stream_lang branch Apr 3, 2013
@elupus

This can cash i think. You are using a string pointer here that the Format call could easily delete before it construct the new one.

Should be:

strItem.AppendFormat(" (%i/%i)", i + 1, (int)setting.max + 1);
@elupus
Member

okay, will change that while investigating the other issue regarding the indices. But it might take some days, or do you think it's urgent?

Member

Nah probably not. It would have given crashes directly if it was a real problem. Should be fixed thou.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment