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

[confluence] add pcm, pcm_s16le and pcm_s24le audio flags #6823

Merged
merged 1 commit into from
Mar 26, 2015

Conversation

un1versal
Copy link
Contributor

This solves no audio flag being present when we playing wav files that contain pcm stream now that most is handled via ffmpeg and not whatever we use to have in kodi, as I remember those use to be ok back then.

  • A example of such is where we use the _audioencoder.wav_ addon to rip a cd to wav which results in _pcm_s16le_ stream.
  • Playing a cd directly the stream is _pcm_ audio and also has no flag so adding it here seems the logical thing to do.
  • When I tested and external ripper to wav the stream was _pcm_s24le_ and also no flag in confluence. this fixes that also.

Without PR these 3 look like screenshot below.

screenshot092 png

This is playing a externally ripped file to wav with pcm_s24le

screenshot002

This is after playing a CD

screenshot000

There maybe others that have no flags in confluence on playback but for time being these are now handled via this PR and display expected flag.

@fritsch @ronie

@un1versal un1versal changed the title [confluence] add pcm_s16le and pcm_s24le flags [confluence] add pcm, pcm_s16le and pcm_s24le audio flags Mar 26, 2015
@un1versal
Copy link
Contributor Author

Adding a reference to List of formats as id by ffmpeg in case someone can spot more that are potentially common enough to justify making flags for them.

un1versal pushed a commit to un1versal/revamped.themes that referenced this pull request Mar 26, 2015
The following were added to default skin

DefaultAddonContextItem
DefaultMusicSearch

The following flags are missing in main skin and revamp(ed)themes

see xbmc/xbmc#6823

pcm
pcm_s16le
pcm_s24le

Default kodi source code references a DeafultRemovableDisk.png which
default skin does not have, here we add that icon for testing where this
is used and if so we add it.

Updated the compressed xbt for usage release to follow.
@ronie
Copy link
Member

ronie commented Mar 26, 2015

looks good to me. or does someone prefer a 'wav' flag instead of 'pcm' for this?

@jjd-uk
Copy link
Member

jjd-uk commented Mar 26, 2015

"WAV" is a container and we already have an icon for that https://github.com/xbmc/xbmc/blob/master/addons/skin.confluence/media/flagging/audio/wav.png

So shouldn't we display both the "WAV" and "PCM" flag

@ronie
Copy link
Member

ronie commented Mar 26, 2015

i know. so which one do you prefer?

@jjd-uk
Copy link
Member

jjd-uk commented Mar 26, 2015

Edited previous post.

Is it possible to show both, as it's also valid to have "DTS" in a "WAV"?

@fritsch
Copy link
Member

fritsch commented Mar 26, 2015

You really want to see WAV next to DTS? That's not really of enduser
interest? The devs sees that in the log.

2015-03-26 17:39 GMT+01:00 jjd-uk notifications@github.com:

Edited previous post.

Is it possible to show both, as it'a also valid to have "DTS" in a "WAV"?

Reply to this email directly or view it on GitHub
#6823 (comment).

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@jjd-uk
Copy link
Member

jjd-uk commented Mar 26, 2015

Not that bothered just raising the point. So sticking with the contents in this case "PCM" is fine by me.

ronie added a commit that referenced this pull request Mar 26, 2015
[confluence] add pcm, pcm_s16le and pcm_s24le audio flags
@ronie ronie merged commit 6a5e763 into xbmc:master Mar 26, 2015
@un1versal
Copy link
Contributor Author

Well, I decided to go with pcm cause indeed it is pcm as per codec identification of both ffmpeg and mediainfo.

@ronie, given we dont display containers I think the wav flag can be deleted as it wont ever show up now ffmpeg is handling this, same would apply to other containers I would think as it will be the stream/codec that shows up and thats what we should show the user.
I guess this issue will occur elsewhere when the container isn't displayed and the audio flag name doesn't match the codec/stream inside, no?

@Jalle19
Copy link
Member

Jalle19 commented Mar 26, 2015

WAV might be more user-friendly, normal people don't know what PCM is.

un1versal pushed a commit to un1versal/revamped.themes that referenced this pull request Mar 27, 2015
The following were added to default skin

DefaultAddonContextItem
DefaultMusicSearch
pcm
pcm_s16le
pcm_s24le

see xbmc/xbmc#6823

Default kodi source code references a DeafultRemovableDisk.png which
default skin does not have, here we add that icon.

Itis used for removable drives like e.g. USB disks.

Updated the compressed xbt for usage release to follow.
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.

None yet

6 participants