I confirm this worked relatively recently.

Created attachment 320728 [details] [review]
fix opus mono->stereo upmixing

And indeed this removes my other patch.
1->2, 2->1, and playing the matroska sample from the cloned bug all work now.

Yes, I can confirm that the patch (combined with the patches provided in #761489) solves the problem! I am able to play the sample file and a couple of others which were generated the same way. Thx!

Created attachment 320893 [details] [review]
guard against empty caps

Just adding an additional check for empty caps before taking the first structure.

Comment on attachment 320893 [details] [review]
guard against empty caps

Please squash that with the other commit. You actually removed the empty check in that other commit :) (gst_caps_get_size() == 0, which is an ugly check for empty caps).

Review of attachment 320728 [details] [review]:

::: ext/opus/gstopusdec.c
@@ +239,2 @@
     caps = gst_caps_truncate (caps);
     caps = gst_caps_make_writable (caps);

If I see this correctly, we would now also allow outputting stereo for 5.1 input and the other way around?

@@ +399,3 @@
+        && dec->n_stereo_streams == 0) {
+      // if we are automatically decoding 2 channels, but only have
+      // a single encoded one, direct both channels to it

No && comments please :)

@@ +841,3 @@
+      GstStructure *s = gst_caps_get_structure (filter, n);
+      if (gst_structure_get_int (s, "channels", &channels)) {
+        if (channels == 1 || channels == 2) {

Even without a filter something can be done here. In the resulting caps we should be able to set channels to [1,2] too even if downstream only supports one of the two (as we can convert in the decoder)

(In reply to Sebastian Dröge (slomo) from comment #6)
> Review of attachment 320728 [details] [review] [review]:
> 
> ::: ext/opus/gstopusdec.c
> @@ +239,2 @@
>      caps = gst_caps_truncate (caps);
>      caps = gst_caps_make_writable (caps);
> 
> If I see this correctly, we would now also allow outputting stereo for 5.1
> input and the other way around?

This will fail negotiation due to different number of channels, as there's just a special case to allow 1 and 2 together. I think all conversions may be legal, in which case, we might just remove that special case and look at how some other such element does. I'm pretty sure there's an element that does channel duplication already which I could copy the logic from.

> @@ +841,3 @@
> +      GstStructure *s = gst_caps_get_structure (filter, n);
> +      if (gst_structure_get_int (s, "channels", &channels)) {
> +        if (channels == 1 || channels == 2) {
> 
> Even without a filter something can be done here. In the resulting caps we
> should be able to set channels to [1,2] too even if downstream only supports
> one of the two (as we can convert in the decoder)

Do you have a test case I can use to make sure this works (getting a call without a filter) ?

(In reply to Vincent Penquerc'h from comment #7)
> (In reply to Sebastian Dröge (slomo) from comment #6)
> > Review of attachment 320728 [details] [review] [review] [review]:
> > 
> > ::: ext/opus/gstopusdec.c
> > @@ +239,2 @@
> >      caps = gst_caps_truncate (caps);
> >      caps = gst_caps_make_writable (caps);
> > 
> > If I see this correctly, we would now also allow outputting stereo for 5.1
> > input and the other way around?
> 
> This will fail negotiation due to different number of channels, as there's
> just a special case to allow 1 and 2 together. I think all conversions may
> be legal, in which case, we might just remove that special case and look at
> how some other such element does. I'm pretty sure there's an element that
> does channel duplication already which I could copy the logic from.

I would keep this as a special case for mono/stereo. Let everything else fail :)

> > @@ +841,3 @@
> > +      GstStructure *s = gst_caps_get_structure (filter, n);
> > +      if (gst_structure_get_int (s, "channels", &channels)) {
> > +        if (channels == 1 || channels == 2) {
> > 
> > Even without a filter something can be done here. In the resulting caps we
> > should be able to set channels to [1,2] too even if downstream only supports
> > one of the two (as we can convert in the decoder)
> 
> Do you have a test case I can use to make sure this works (getting a call
> without a filter) ?

Without writing code? No. Do a caps query on the sinkpad without a filter, and have downstream a capsfilter that accepts only mono or only stereo.

Vincent?

Here! :) I'll get back to finishing those Opus patches when I get a bit of time.

What's your ETA for that? We should try to get this fixed before 1.8.0 as it's a regression :) I was planning to do 1.7.90 some time later this week.

Not sure, but I can probably find time before the end of the week if required. But what's left is the filterless case, is that part a regression since it needs custom code to trigger ?

The filter-less code should've worked before already so yes. That part actually seems easy to fix, no?


The more important part is to only limit this channel remixing feature to mono/stereo instead of allowing it for arbitrary channel layouts. It's going to cause problems for others and behave differently than audioconvert.

OK, if it's easy to fix I'll try to get on it soon :)

Not really a blocker. Opus is in -bad currently so we can change behaviour, and it does not really seem like a feature that is really needed. You can always use audioconvert.

Created attachment 322355 [details] [review]
320728: fix opus mono->stereo upmixing

Works with 1/2 and 2/1, fails with >2/<=2 and <=2/>2.

Caps query with NULL filter does the right thing (proxied downstream).

Comment and squash done.

Comment on attachment 322355 [details] [review]
320728: fix opus mono->stereo upmixing

Does not apply to latest GIT master.

Also this only prevents conversions for >2 channels in get_caps(). What if downstream has "audio/x-raw,channels=2; audio/x-raw,channels=5" and you give 5 channels as input. Is it going to negotiate 2 channels then because that's what gst_caps_fixate() would yield?

Created attachment 322363 [details] [review]
320728: fix opus mono->stereo upmixing

Oops. Rebased to latest master.

(In reply to Sebastian Dröge (slomo) from comment #17)

> Also this only prevents conversions for >2 channels in get_caps(). What if
> downstream has "audio/x-raw,channels=2; audio/x-raw,channels=5" and you give
> 5 channels as input. Is it going to negotiate 2 channels then because that's
> what gst_caps_fixate() would yield?

I'll try that and get back to you :)

And yes, it fails instead of allowing 5.

Comment on attachment 322363 [details] [review]
320728: fix opus mono->stereo upmixing

Ok, but that shouldn't fail :)

Created attachment 322373 [details] [review]
fix opus mono->stereo upmixing

Fixed :)

I now iterate through the structures till I find one that fits the format that was set, if any. Still works with 1->2, 2->1, and playing that matroska file from the original bug.

Review of attachment 322373 [details] [review]:

Why the change for opusenc btw? This shouldn't be needed and seems incorrect. It's the decoder that can downsample/downmix, not the encoder

::: ext/opus/gstopusdec.c
@@ +250,3 @@
+      else
+        gst_structure_set (s, "rate", G_TYPE_INT, dec->sample_rate, NULL);
+      gst_structure_get_int (s, "rate", &rate);

We can always output 48kHz, but if the input gives a different sample rate we would prefer that one while still supporting to output 48kHz. Is this considered here or will this only allow 48kHz if no sample rate on input?

@@ +254,1 @@
+      if (dec->sample_rate > 0 && dec->sample_rate != rate) {

If dec->sample_rate == 0, you would above set 0 inside the caps. That doesn't seem right

@@ +254,3 @@
+      if (dec->sample_rate > 0 && dec->sample_rate != rate) {
+        GST_WARNING_OBJECT (dec, "Don't like rate of %u, wanted %u", rate,
+            dec->sample_rate);

Not a warning really, just normal negotiation happening here

@@ +268,3 @@
+        if (dec->n_channels > 2 || channels > 2) {
+          GST_WARNING_OBJECT (dec, "Don't like channels of %u, wanted %u",
+              channels, dec->n_channels);

Same here

(In reply to Sebastian Dröge (slomo) from comment #23)
> Review of attachment 322373 [details] [review] [review]:
> 
> Why the change for opusenc btw? This shouldn't be needed and seems
> incorrect. It's the decoder that can downsample/downmix, not the encoder

Without it, the proxied caps come back as EMPTY, so that might imply a bug in the proxying ? Both input caps and filter contain channels=2 (this is a test with 1 channel upstream and 2 channels downstream).

I'll fix the rest.

Created attachment 322389 [details] [review]
fix opus mono->stereo upmixing

All done, the bit about changing 1 or 2 to [1,2] that was in enc is now moved to dec. Everything still seems to work fine :)

Review of attachment 322389 [details] [review]:

And only a change in opusdec this time \o/

::: ext/opus/gstopusdec.c
@@ +261,3 @@
+          }
+        }
+      }

This might need an else case where we explicitly check for 48000 only and ignore the structure otherwise we might select below a channel setup that is not going to work

@@ +279,3 @@
+          }
+        }
+      }

And also an else case here I guess? What is if n_channels==0? What would we do then later? Stereo? If so we should check for stereo here

@@ +284,3 @@
+      dec->n_channels = channels;
+      break;
+    }

All this loop might be possible to do easier with gst_caps_intersect() btw. You could create new caps with only rate (if ->sample_rate != 0 then {x, 48000} else 48000) and channels (if ->n_channels ...). And then just check the result of the intersection with that. Or am I missing something?

@@ +884,3 @@
+    gst_caps_unref (filter);
+  if (caps) {
+    caps = gst_caps_copy (caps);

caps = gst_caps_make_writable(caps);

(you leak the original caps here and also make unnecessary copies most of the time)

(In reply to Sebastian Dröge (slomo) from comment #26)
> Review of attachment 322389 [details] [review] [review]:
> ::: ext/opus/gstopusdec.c
> @@ +261,3 @@
> +          }
> +        }
> +      }
> 
> This might need an else case where we explicitly check for 48000 only and
> ignore the structure otherwise we might select below a channel setup that is
> not going to work

I do not understand.

> And also an else case here I guess? What is if n_channels==0? What would we
> do then later? Stereo? If so we should check for stereo here

This is checked below, and default values set if nothing could be matched. This may end up failing, probably.

> All this loop might be possible to do easier with gst_caps_intersect() btw.
> You could create new caps with only rate (if ->sample_rate != 0 then {x,
> 48000} else 48000) and channels (if ->n_channels ...). And then just check
> the result of the intersection with that. Or am I missing something?

I think that'd work (intersect, then truncate, then fixate). The intersection would have got rid of the 1/5 case you mentioned earlier.

Ok, if you do the intersection then also my other comments are meaningless :)

Created attachment 322451 [details] [review]
fix opus mono->stereo upmixing

Review of attachment 322451 [details] [review]:

::: ext/opus/gstopusdec.c
@@ +241,3 @@
+    if (dec->sample_rate == 0) {
+      if (dec->n_channels == 0) {
+        str = g_strdup ("audio/x-raw");

If channels==0, don't we only support mono/stereo then? Or what do we do?

@@ +246,3 @@
+      } else {
+        str = g_strdup_printf ("audio/x-raw, channels=[%u]", dec->n_channels);
+      }

If sample_rate == 0 we only support 48kHz

@@ +262,2 @@
     }
+    constraint = gst_caps_from_string (str);

Why string programming?

caps = gst_caps_new_empty_simple("audio/x-raw");
if (dec->sample_rate == 0) {
  ...
  gst_caps_set_simple(caps, "channels", GST_TYPE_INT_RANGE, 1, 2, NULL);
}

etc

@@ +272,1 @@
       return;

Why doesn't this function return a boolean and propagate failures upwards?

(In reply to Sebastian Dröge (slomo) from comment #30)
> Review of attachment 322451 [details] [review] [review]:
> 
> ::: ext/opus/gstopusdec.c
> @@ +241,3 @@
> +    if (dec->sample_rate == 0) {
> +      if (dec->n_channels == 0) {
> +        str = g_strdup ("audio/x-raw");
> 
> If channels==0, don't we only support mono/stereo then? Or what do we do?

It will try to default to stereo below.

Remainder done.

Created attachment 322456 [details] [review]
fix opus mono->stereo upmixing

Review of attachment 322456 [details] [review]:

Ok, thanks :) just one last simple thing I hope

::: ext/opus/gstopusdec.c
@@ +240,3 @@
+    constraint = gst_caps_from_string ("audio/x-raw");
+    if (dec->sample_rate == 0) {
+      if (dec->n_channels == 0) {

You can split these two ifs to reduce indentation.

if (dec->sample_rate == 0) {

}

if (dec->n_channels == 0) {

}

Just call gst_caps_set_simple() twice.

@@ +261,3 @@
+      g_value_unset (&v);
+      g_value_unset (&l);
+      if (dec->n_channels == 0) {

It will default to stereo below, but shouldn't we ideally check here then if downstream accepts stereo? And for consistency, should we go mono or stereo, depending on what downstream accepts (i.e. go into the <= 2 case too), and prefer stereo? (i.e. fixate_nearest_int(2) further below).

Created attachment 322464 [details] [review]
fix opus mono->stereo upmixing

Review of attachment 322464 [details] [review]:

::: ext/opus/gstopusdec.c
@@ +256,2 @@
     }
+    if (dec->n_channels == 0) {

Shouldn't this case go away and become the <= 2 one?

@@ +392,3 @@
 
+  if (!gst_opus_dec_negotiate (dec, posn))
+    return GST_FLOW_ERROR;

GST_FLOW_NOT_NEGOTIATED

@@ +433,3 @@
 
+      if (!gst_opus_dec_negotiate (dec, NULL))
+        return GST_FLOW_ERROR;

GST_FLOW_NOT_NEGOTIATED

(In reply to Sebastian Dröge (slomo) from comment #35)
> Review of attachment 322464 [details] [review] [review]:
> 
> ::: ext/opus/gstopusdec.c
> @@ +256,2 @@
>      }
> +    if (dec->n_channels == 0) {
> 
> Shouldn't this case go away and become the <= 2 one?

If downstream only supports 4 channels, that'd prevent negotiation, no ?

Created attachment 322470 [details] [review]
fix opus mono->stereo upmixing

(In reply to Vincent Penquerc'h from comment #36)
> (In reply to Sebastian Dröge (slomo) from comment #35)
> > Review of attachment 322464 [details] [review] [review] [review]:
> > 
> > ::: ext/opus/gstopusdec.c
> > @@ +256,2 @@
> >      }
> > +    if (dec->n_channels == 0) {
> > 
> > Shouldn't this case go away and become the <= 2 one?
> 
> If downstream only supports 4 channels, that'd prevent negotiation, no ?

Yes, but it should. Because we would negotiate 2 channels and that would then fail much later anyway. Without channels in the caps we can only support mono or stereo.

Review of attachment 322470 [details] [review]:

Just that one more thing, then please push :) Or explain why you think the current channels==0 handling in the patch is correct

::: ext/opus/gstopusdec.c
@@ +278,1 @@
     if (gst_structure_has_field (s, "rate"))

By construction, s always has the rate field now btw :)

@@ +284,1 @@
     if (gst_structure_has_field (s, "channels"))

And once you fixed the channels part it will have that field too always

(In reply to Sebastian Dröge (slomo) from comment #38)
> > If downstream only supports 4 channels, that'd prevent negotiation, no ?
> 
> Yes, but it should. Because we would negotiate 2 channels and that would
> then fail much later anyway. Without channels in the caps we can only
> support mono or stereo.

I see, I'll change it.

Created attachment 322475 [details] [review]
fix opus mono->stereo upmixing

Review of attachment 322475 [details] [review]:

Ok, let's go ahead with this :) Thanks!

I've squashed it with the previous, which I'm just seeing was not pushed yet, and it replaces it anyway.

commit 625bd68da71e73369f14774d3115dedd7eb6ff64
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Thu Feb 4 16:01:00 2016 +0000

    opus: fix mono<->stereo up/down-mixing
    
    https://bugzilla.gnome.org/show_bug.cgi?id=761588