All of lore.kernel.org
 help / color / mirror / Atom feed
* Duplicated/weird SND_CHMAP_xx positions
@ 2013-11-10 16:19 Anssi Hannula
  2013-11-10 19:24 ` [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH Anssi Hannula
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Anssi Hannula @ 2013-11-10 16:19 UTC (permalink / raw)
  To: alsa-devel, Takashi Iwai

Hi,

I see we have both:
        SND_CHMAP_FLH,          /**< front left high */
        SND_CHMAP_FCH,          /**< front center high */
        SND_CHMAP_FRH,          /**< front right high */
and
        SND_CHMAP_TFL,          /**< top front left */
        SND_CHMAP_TFR,          /**< top front right */
        SND_CHMAP_TFC,          /**< top front center */

However, these are actually exactly the same AFAICS, both are the
speakers above the front speakers (CEA/HDMI uses "high" and USB audio
uses "top" nomenclature).

I guess we should deprecate one set?
I can't think of anything else than adding comments saying they are
deprecated, though...

I guess I'd prefer to deprecate the "high" ones since we have many other
"top" and "bottom" channels.

Also, the only in-kernel use of "high" is by HDA HDMI and it only will
start working in 3.13-rcX.


We have also these:
        SND_CHMAP_BC,           /**< bottom center */
        SND_CHMAP_BLC,          /**< bottom left center */
        SND_CHMAP_BRC,          /**< bottom right center */

I guess "bottom center" is "under the listener" (USB audio v2 spec
doesn't clarify, but clearly doesn't use the "Front" specifier as in
"front bottom center").

However, the last two ones are weird. The descriptions do not make sense
to me (under the listener to the left? wouldn't that be "bottom side
left" instead?).

It seems like they were added by mistake - the only user is the USB
audio driver which wrongly uses them for "back left/right center"
instead of correct RLC/RRC.



For reference, the complete current position list is:
/** channel positions */
enum snd_pcm_chmap_position {
        SND_CHMAP_UNKNOWN = 0,  /**< unspecified */
        SND_CHMAP_NA,           /**< N/A, silent */
        SND_CHMAP_MONO,         /**< mono stream */
        SND_CHMAP_FL,           /**< front left */
        SND_CHMAP_FR,           /**< front right */
        SND_CHMAP_RL,           /**< rear left */
        SND_CHMAP_RR,           /**< rear right */
        SND_CHMAP_FC,           /**< front center */
        SND_CHMAP_LFE,          /**< LFE */
        SND_CHMAP_SL,           /**< side left */
        SND_CHMAP_SR,           /**< side right */
        SND_CHMAP_RC,           /**< rear center */
        SND_CHMAP_FLC,          /**< front left center */
        SND_CHMAP_FRC,          /**< front right center */
        SND_CHMAP_RLC,          /**< rear left center */
        SND_CHMAP_RRC,          /**< rear right center */
        SND_CHMAP_FLW,          /**< front left wide */
        SND_CHMAP_FRW,          /**< front right wide */
        SND_CHMAP_FLH,          /**< front left high */
        SND_CHMAP_FCH,          /**< front center high */
        SND_CHMAP_FRH,          /**< front right high */
        SND_CHMAP_TC,           /**< top center */
        SND_CHMAP_TFL,          /**< top front left */
        SND_CHMAP_TFR,          /**< top front right */
        SND_CHMAP_TFC,          /**< top front center */
        SND_CHMAP_TRL,          /**< top rear left */
        SND_CHMAP_TRR,          /**< top rear right */
        SND_CHMAP_TRC,          /**< top rear center */
        SND_CHMAP_TFLC,         /**< top front left center */
        SND_CHMAP_TFRC,         /**< top front right center */
        SND_CHMAP_TSL,          /**< top side left */
        SND_CHMAP_TSR,          /**< top side right */
        SND_CHMAP_LLFE,         /**< left LFE */
        SND_CHMAP_RLFE,         /**< right LFE */
        SND_CHMAP_BC,           /**< bottom center */
        SND_CHMAP_BLC,          /**< bottom left center */
        SND_CHMAP_BRC,          /**< bottom right center */
        SND_CHMAP_LAST = SND_CHMAP_BRC,
};


-- 
Anssi Hannula

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH
  2013-11-10 16:19 Duplicated/weird SND_CHMAP_xx positions Anssi Hannula
@ 2013-11-10 19:24 ` Anssi Hannula
  2013-11-11 16:07   ` Takashi Iwai
  2013-11-10 19:24 ` [PATCH 2/2] ALSA: usb: Fix wrong mapping of RLC and RRC channels Anssi Hannula
  2013-11-11 16:05 ` Duplicated/weird SND_CHMAP_xx positions Takashi Iwai
  2 siblings, 1 reply; 6+ messages in thread
From: Anssi Hannula @ 2013-11-10 19:24 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Channel map positions FLH, FCH, FRH duplicate positions TFL, TFC, TFR.
Both are the speakers above the front speakers (CEA uses "high" and USB
audio uses "top" nomenclature).

Since the USB audio code has used the TFx positions since v3.8
(04324ccc75f96, "ALSA: usb-audio: add channel map support") but the HDMI
code only just started using FxH in a5b7d510b2220cccb ("ALSA: hda -
hdmi: Fix channel maps with less common speakers") which is not yet in
any released kernel, standardize on TFx instead.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
---
 sound/pci/hda/patch_hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index fe3b150f97d9..4ad8c055c221 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -763,12 +763,12 @@ static struct channel_map_table map_tables[] = {
 	{ SNDRV_CHMAP_RC,	RC },
 	{ SNDRV_CHMAP_FLC,	FLC },
 	{ SNDRV_CHMAP_FRC,	FRC },
-	{ SNDRV_CHMAP_FLH,	FLH },
-	{ SNDRV_CHMAP_FRH,	FRH },
+	{ SNDRV_CHMAP_TFL,	FLH },
+	{ SNDRV_CHMAP_TFR,	FRH },
 	{ SNDRV_CHMAP_FLW,	FLW },
 	{ SNDRV_CHMAP_FRW,	FRW },
 	{ SNDRV_CHMAP_TC,	TC },
-	{ SNDRV_CHMAP_FCH,	FCH },
+	{ SNDRV_CHMAP_TFC,	FCH },
 	{} /* terminator */
 };
 
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ALSA: usb: Fix wrong mapping of RLC and RRC channels
  2013-11-10 16:19 Duplicated/weird SND_CHMAP_xx positions Anssi Hannula
  2013-11-10 19:24 ` [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH Anssi Hannula
@ 2013-11-10 19:24 ` Anssi Hannula
  2013-11-11 16:05 ` Duplicated/weird SND_CHMAP_xx positions Takashi Iwai
  2 siblings, 0 replies; 6+ messages in thread
From: Anssi Hannula @ 2013-11-10 19:24 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

According to USB Audio spec v2 bits 25 and 26 of bmChannelConfig are
"Back Left of Center - BLC" and "Back Right of Center - BRC",
respectively.

They are currently assigned to ALSA channels BLC/BRC. However, the ALSA
BLC/BRC are actually the rather nonsensical "bottom left center" and
"bottom right center", so the channels will be assigned wrongly. The
comments in the USB code are also similarly wrong, so this is not
readily apparent without looking at the actual specification.

Fix the channel mapping by mapping bits 25 and 26 to RLC (Rear Left
Center) and RRC (Rear Right Center), respectively, instead.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
---
 sound/usb/stream.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index d737d0e6e558..2fb71be5e100 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -273,8 +273,8 @@ static struct snd_pcm_chmap_elem *convert_chmap(int channels, unsigned int bits,
 		SNDRV_CHMAP_TSL,	/* top side left */
 		SNDRV_CHMAP_TSR,	/* top side right */
 		SNDRV_CHMAP_BC,		/* bottom center */
-		SNDRV_CHMAP_BLC,	/* bottom left center */
-		SNDRV_CHMAP_BRC,	/* bottom right center */
+		SNDRV_CHMAP_RLC,	/* back left of center */
+		SNDRV_CHMAP_RRC,	/* back right of center */
 		0 /* terminator */
 	};
 	struct snd_pcm_chmap_elem *chmap;
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: Duplicated/weird SND_CHMAP_xx positions
  2013-11-10 16:19 Duplicated/weird SND_CHMAP_xx positions Anssi Hannula
  2013-11-10 19:24 ` [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH Anssi Hannula
  2013-11-10 19:24 ` [PATCH 2/2] ALSA: usb: Fix wrong mapping of RLC and RRC channels Anssi Hannula
@ 2013-11-11 16:05 ` Takashi Iwai
  2013-11-14  0:56   ` Anssi Hannula
  2 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2013-11-11 16:05 UTC (permalink / raw)
  To: Anssi Hannula; +Cc: alsa-devel

At Sun, 10 Nov 2013 18:19:26 +0200,
Anssi Hannula wrote:
> 
> Hi,
> 
> I see we have both:
>         SND_CHMAP_FLH,          /**< front left high */
>         SND_CHMAP_FCH,          /**< front center high */
>         SND_CHMAP_FRH,          /**< front right high */
> and
>         SND_CHMAP_TFL,          /**< top front left */
>         SND_CHMAP_TFR,          /**< top front right */
>         SND_CHMAP_TFC,          /**< top front center */
> 
> However, these are actually exactly the same AFAICS, both are the
> speakers above the front speakers (CEA/HDMI uses "high" and USB audio
> uses "top" nomenclature).

Oh, a good catch.

> I guess we should deprecate one set?
> I can't think of anything else than adding comments saying they are
> deprecated, though...

Yeah, commenting would be the best effort...

> I guess I'd prefer to deprecate the "high" ones since we have many other
> "top" and "bottom" channels.

Indeed, TOP elements are already used in the recent patch in
gstreamer, so we should keep them.

> Also, the only in-kernel use of "high" is by HDA HDMI and it only will
> start working in 3.13-rcX.

Right, and this can be fixed in 3.13 cycle.

> We have also these:
>         SND_CHMAP_BC,           /**< bottom center */
>         SND_CHMAP_BLC,          /**< bottom left center */
>         SND_CHMAP_BRC,          /**< bottom right center */
> 
> I guess "bottom center" is "under the listener" (USB audio v2 spec
> doesn't clarify, but clearly doesn't use the "Front" specifier as in
> "front bottom center").
> 
> However, the last two ones are weird. The descriptions do not make sense
> to me (under the listener to the left? wouldn't that be "bottom side
> left" instead?).
>
> It seems like they were added by mistake - the only user is the USB
> audio driver which wrongly uses them for "back left/right center"
> instead of correct RLC/RRC.

These are taken from gstreamer definitions.  I'm not quite sure about
these distinctions, but added just for completeness in any future use
case.  So, just keep these unused for now.


Takashi


> For reference, the complete current position list is:
> /** channel positions */
> enum snd_pcm_chmap_position {
>         SND_CHMAP_UNKNOWN = 0,  /**< unspecified */
>         SND_CHMAP_NA,           /**< N/A, silent */
>         SND_CHMAP_MONO,         /**< mono stream */
>         SND_CHMAP_FL,           /**< front left */
>         SND_CHMAP_FR,           /**< front right */
>         SND_CHMAP_RL,           /**< rear left */
>         SND_CHMAP_RR,           /**< rear right */
>         SND_CHMAP_FC,           /**< front center */
>         SND_CHMAP_LFE,          /**< LFE */
>         SND_CHMAP_SL,           /**< side left */
>         SND_CHMAP_SR,           /**< side right */
>         SND_CHMAP_RC,           /**< rear center */
>         SND_CHMAP_FLC,          /**< front left center */
>         SND_CHMAP_FRC,          /**< front right center */
>         SND_CHMAP_RLC,          /**< rear left center */
>         SND_CHMAP_RRC,          /**< rear right center */
>         SND_CHMAP_FLW,          /**< front left wide */
>         SND_CHMAP_FRW,          /**< front right wide */
>         SND_CHMAP_FLH,          /**< front left high */
>         SND_CHMAP_FCH,          /**< front center high */
>         SND_CHMAP_FRH,          /**< front right high */
>         SND_CHMAP_TC,           /**< top center */
>         SND_CHMAP_TFL,          /**< top front left */
>         SND_CHMAP_TFR,          /**< top front right */
>         SND_CHMAP_TFC,          /**< top front center */
>         SND_CHMAP_TRL,          /**< top rear left */
>         SND_CHMAP_TRR,          /**< top rear right */
>         SND_CHMAP_TRC,          /**< top rear center */
>         SND_CHMAP_TFLC,         /**< top front left center */
>         SND_CHMAP_TFRC,         /**< top front right center */
>         SND_CHMAP_TSL,          /**< top side left */
>         SND_CHMAP_TSR,          /**< top side right */
>         SND_CHMAP_LLFE,         /**< left LFE */
>         SND_CHMAP_RLFE,         /**< right LFE */
>         SND_CHMAP_BC,           /**< bottom center */
>         SND_CHMAP_BLC,          /**< bottom left center */
>         SND_CHMAP_BRC,          /**< bottom right center */
>         SND_CHMAP_LAST = SND_CHMAP_BRC,
> };
> 
> 
> -- 
> Anssi Hannula
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH
  2013-11-10 19:24 ` [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH Anssi Hannula
@ 2013-11-11 16:07   ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2013-11-11 16:07 UTC (permalink / raw)
  To: Anssi Hannula; +Cc: alsa-devel

At Sun, 10 Nov 2013 21:24:04 +0200,
Anssi Hannula wrote:
> 
> Channel map positions FLH, FCH, FRH duplicate positions TFL, TFC, TFR.
> Both are the speakers above the front speakers (CEA uses "high" and USB
> audio uses "top" nomenclature).
> 
> Since the USB audio code has used the TFx positions since v3.8
> (04324ccc75f96, "ALSA: usb-audio: add channel map support") but the HDMI
> code only just started using FxH in a5b7d510b2220cccb ("ALSA: hda -
> hdmi: Fix channel maps with less common speakers") which is not yet in
> any released kernel, standardize on TFx instead.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>

Applied both patches now.  Thanks.


Takashi

> ---
>  sound/pci/hda/patch_hdmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index fe3b150f97d9..4ad8c055c221 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -763,12 +763,12 @@ static struct channel_map_table map_tables[] = {
>  	{ SNDRV_CHMAP_RC,	RC },
>  	{ SNDRV_CHMAP_FLC,	FLC },
>  	{ SNDRV_CHMAP_FRC,	FRC },
> -	{ SNDRV_CHMAP_FLH,	FLH },
> -	{ SNDRV_CHMAP_FRH,	FRH },
> +	{ SNDRV_CHMAP_TFL,	FLH },
> +	{ SNDRV_CHMAP_TFR,	FRH },
>  	{ SNDRV_CHMAP_FLW,	FLW },
>  	{ SNDRV_CHMAP_FRW,	FRW },
>  	{ SNDRV_CHMAP_TC,	TC },
> -	{ SNDRV_CHMAP_FCH,	FCH },
> +	{ SNDRV_CHMAP_TFC,	FCH },
>  	{} /* terminator */
>  };
>  
> -- 
> 1.8.1.5
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Duplicated/weird SND_CHMAP_xx positions
  2013-11-11 16:05 ` Duplicated/weird SND_CHMAP_xx positions Takashi Iwai
@ 2013-11-14  0:56   ` Anssi Hannula
  0 siblings, 0 replies; 6+ messages in thread
From: Anssi Hannula @ 2013-11-14  0:56 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

11.11.2013 18:05, Takashi Iwai kirjoitti:
> At Sun, 10 Nov 2013 18:19:26 +0200,
> Anssi Hannula wrote:
[...]
>> We have also these:
>>         SND_CHMAP_BC,           /**< bottom center */
>>         SND_CHMAP_BLC,          /**< bottom left center */
>>         SND_CHMAP_BRC,          /**< bottom right center */
>>
>> I guess "bottom center" is "under the listener" (USB audio v2 spec
>> doesn't clarify, but clearly doesn't use the "Front" specifier as in
>> "front bottom center").
>>
>> However, the last two ones are weird. The descriptions do not make sense
>> to me (under the listener to the left? wouldn't that be "bottom side
>> left" instead?).
>>
>> It seems like they were added by mistake - the only user is the USB
>> audio driver which wrongly uses them for "back left/right center"
>> instead of correct RLC/RRC.
> 
> These are taken from gstreamer definitions.  I'm not quite sure about
> these distinctions, but added just for completeness in any future use
> case.  So, just keep these unused for now.

gst-1.0 has:
  GST_AUDIO_CHANNEL_POSITION_BOTTOM_FRONT_CENTER,
  GST_AUDIO_CHANNEL_POSITION_BOTTOM_FRONT_LEFT,
  GST_AUDIO_CHANNEL_POSITION_BOTTOM_FRONT_RIGHT,

So they are clearly labelled "front" and only the first one is "center".

Your gst patch (at least the ml version I saw) mapped them like this:
+  ITEM(BC, BOTTOM_FRONT_CENTER),
+  ITEM(BLC, BOTTOM_FRONT_LEFT),
+  ITEM(BRC, BOTTOM_FRONT_LEFT),

But I was under the assumption that BC is under the listener instead of
front center, since otherwise it would be "BFC", not "BC".
(though I don't find it unlikely at all that "bottom-center" got to the
USB spec by a mistake... but if it were just a case of missing "front"
specifier, one'd think the spec would have "bottom-front-left" and
"bottom-front-right" as well, which it does not)

Also, clearly BLC is not bottom front left, since the 'C' stands for
"center". Not to mention BRC being right, not left. ;)

So I guess the nonsensical BLC/BRC was just a mistake then... I guess we
can pretend they are between BC and BSL/BSR (bottom side left/right) or
something.

>> For reference, the complete current position list is:
>> /** channel positions */
>> enum snd_pcm_chmap_position {
>>         SND_CHMAP_UNKNOWN = 0,  /**< unspecified */
>>         SND_CHMAP_NA,           /**< N/A, silent */
>>         SND_CHMAP_MONO,         /**< mono stream */
>>         SND_CHMAP_FL,           /**< front left */
>>         SND_CHMAP_FR,           /**< front right */
>>         SND_CHMAP_RL,           /**< rear left */
>>         SND_CHMAP_RR,           /**< rear right */
>>         SND_CHMAP_FC,           /**< front center */
>>         SND_CHMAP_LFE,          /**< LFE */
>>         SND_CHMAP_SL,           /**< side left */
>>         SND_CHMAP_SR,           /**< side right */
>>         SND_CHMAP_RC,           /**< rear center */
>>         SND_CHMAP_FLC,          /**< front left center */
>>         SND_CHMAP_FRC,          /**< front right center */
>>         SND_CHMAP_RLC,          /**< rear left center */
>>         SND_CHMAP_RRC,          /**< rear right center */
>>         SND_CHMAP_FLW,          /**< front left wide */
>>         SND_CHMAP_FRW,          /**< front right wide */
>>         SND_CHMAP_FLH,          /**< front left high */
>>         SND_CHMAP_FCH,          /**< front center high */
>>         SND_CHMAP_FRH,          /**< front right high */
>>         SND_CHMAP_TC,           /**< top center */
>>         SND_CHMAP_TFL,          /**< top front left */
>>         SND_CHMAP_TFR,          /**< top front right */
>>         SND_CHMAP_TFC,          /**< top front center */
>>         SND_CHMAP_TRL,          /**< top rear left */
>>         SND_CHMAP_TRR,          /**< top rear right */
>>         SND_CHMAP_TRC,          /**< top rear center */
>>         SND_CHMAP_TFLC,         /**< top front left center */
>>         SND_CHMAP_TFRC,         /**< top front right center */
>>         SND_CHMAP_TSL,          /**< top side left */
>>         SND_CHMAP_TSR,          /**< top side right */
>>         SND_CHMAP_LLFE,         /**< left LFE */
>>         SND_CHMAP_RLFE,         /**< right LFE */
>>         SND_CHMAP_BC,           /**< bottom center */
>>         SND_CHMAP_BLC,          /**< bottom left center */
>>         SND_CHMAP_BRC,          /**< bottom right center */
>>         SND_CHMAP_LAST = SND_CHMAP_BRC,
>> };
>>
>>
>> -- 
>> Anssi Hannula
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 


-- 
Anssi Hannula

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-11-14  0:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-10 16:19 Duplicated/weird SND_CHMAP_xx positions Anssi Hannula
2013-11-10 19:24 ` [PATCH 1/2] ALSA: hda - hdmi: Use TFx channel positions instead of FxH Anssi Hannula
2013-11-11 16:07   ` Takashi Iwai
2013-11-10 19:24 ` [PATCH 2/2] ALSA: usb: Fix wrong mapping of RLC and RRC channels Anssi Hannula
2013-11-11 16:05 ` Duplicated/weird SND_CHMAP_xx positions Takashi Iwai
2013-11-14  0:56   ` Anssi Hannula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.