I fixed the rate/chanels issue and the warnings about sticky events orderings. But the real issue here is that the caps isn't a subset of the template one:

audio/x-wma, wmaversion=(int)2, block_align=(int)[ 0, 2147483647 ], bitrate=(int)[ 0, 2147483647 ]

audio/x-wma, wmaversion=(int)2, codec_data=(buffer)6201020044ac0000451f0000cf05100012001000030000000000000000000000e00042c0


The docs state that the is_subset function won't work reliably in case there are optional parameters.

Those docs are outdated (and have been updated in git I believe), it works fine now :)

The problem here is the missing block_align and bitrate fields. If the template caps have those, the caps must have those fields too, otherwise it won't be a subset.

I am not sure what the expected/best workaround is here. It *looks* like libav needs these values, so we need to get them somewhere. Maybe we can/have to exract them from the codec data if missing?

Created attachment 244308 [details] [review]
mssdemux: send stream id and newsegment before pushing data

Fixes sticky events ordering warnings when data is pushed

Created attachment 244309 [details] [review]
mssdemux: add bitrate info to audio streams

bitrate info is always present on the QualityLevel xml node as part
of the adaptive selection processing, put it into caps as some
decoders require it (avdec_wmav2 for example)

Created attachment 244310 [details] [review]
mssdemux: parse block_align from waveformatex if possible

wma v2 expects block_align field set to its caps. This isn't present
direclty on the manifests, so mssdemux should parse it from the
waveformatex structure

These patches add the bitrate and block_align entries. Still failing on caps negotiation. Going to investigate it further later today.

This may help:
commit d802c7395aad52aaaa593f5578811c8766437cca
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Wed May 15 17:15:18 2013 +0200

    playback: Only do a subset filtering for the factories if we have fixed caps
    
    Otherwise we're plugging a parser/converter currently and have unfixed caps.

Thanks. The element is now being linked and with a few more caps fixes it starts up. The issue now is that the audio is broken. Still investigating what might be causing it

I pushed the fixes that make the decoder link properly, but still haven't fixed the garbled sound issue.

Thiago, any updates ?

The mss patches were pushed, the problem is still that the audio isn't playing and libav throws lots of

0:00:17.210740755 23508 0xb5804b20 ERROR                  libav :0:: broken frame: channel len > samples_per_frame

during playback. I'm unassigning myself as I'm not the best person to debug this.

Created attachment 257428 [details] [review]
fix setting codec_data when using WaveFormatEx

Hi,

the problem about the audio not playing is that the codec_data passed to the decoder is the whole WaveFormatEx and not just the actual codec private data (which is the data from byte 18 on in WaveFormatEx when the cbSize field is non zero).

The attached patch makes audio work with the bigbuckbunny video from the blender foundation.

The patch is just an ugly hack, as I am not proficient in gstreamer and glib stuff, I'd like someone else to fix it properly or to give me direction about how a final patch should look like.

Thanks,
   Antonio

If we can assume the data in WaveFormatEx is actually always a WAVEFORMATEX[1] structure and never a smaller one like WAVEFORMAT[2] or PCMWAVEFORMAT[3] then we can assume the data is always at least 18 bytes and this will simplify the code, anyone knows why the current check is for >= 14?

Has anyone got the specs of the manifest format?

Or if we really want to handle the 14 and 16 bytes cases we can assume that in those cases there will be no codec private data, would that be OK?

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/dd390970%28v=vs.85%29.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/dd757712%28v=vs.85%29.aspx
[3] http://msdn.microsoft.com/en-us/library/windows/desktop/dd743663%28v=vs.85%29.aspx

Review of attachment 257428 [details] [review]:

::: ext/smoothstreaming/gstmssmanifest.c
@@ +612,3 @@
+
+        if (mapinfo.size >= 16) {
+          gint cbSize = GST_READ_UINT16_LE (mapinfo.data + 16);

You need to check that it's >= 18 if you want to read 2 bytes at offset 16

@@ +613,3 @@
+        if (mapinfo.size >= 16) {
+          gint cbSize = GST_READ_UINT16_LE (mapinfo.data + 16);
+          fprintf (stderr, "%s cbSize: %d\n", __func__, cbSize);

You can use GST_DEBUG() here

@@ +619,3 @@
+             * whole "WaveFormatEx" */
+            gst_buffer_resize (codec_data, 18, -1);
+            goto endif;

Best would be without a goto here :) Also won't you still pass the complete struct as codec_data in all other cases?

(In reply to comment #14)
> Review of attachment 257428 [details] [review]:
> 
> ::: ext/smoothstreaming/gstmssmanifest.c
> @@ +612,3 @@
> +
> +        if (mapinfo.size >= 16) {
> +          gint cbSize = GST_READ_UINT16_LE (mapinfo.data + 16);
> 
> You need to check that it's >= 18 if you want to read 2 bytes at offset 16
>

Of course, thanks, I spotted this just after I had sent out the patch :)

> @@ +613,3 @@
> +        if (mapinfo.size >= 16) {
> +          gint cbSize = GST_READ_UINT16_LE (mapinfo.data + 16);
> +          fprintf (stderr, "%s cbSize: %d\n", __func__, cbSize);
> 
> You can use GST_DEBUG() here
>

I will, thanks, I am still learning about the infrastructure.

> @@ +619,3 @@
> +             * whole "WaveFormatEx" */
> +            gst_buffer_resize (codec_data, 18, -1);
> +            goto endif;
> 
> Best would be without a goto here :) Also won't you still pass the complete
> struct as codec_data in all other cases?

I want to check with the Microsoft smooth streaming specification before sending an update, chances are that the code can just be simplified: if the wav header is _always_ supposed to be a WAVEFORMATEX (at least 18 bytes), than I'll always consume these 18 bytes and just pass the rest to the decoder in codec_data.

Regards,
   Antonio

Created attachment 258677 [details] [review]
mssdemux: fix setting codec_data when using WaveFormatEx

OK, the smoothstreaming specifications[1,2,3] always mention WAVEFORMATEX
and never smaller structures like WAVEFORMAT or PCMWAVEFORMAT, so I think the
buffer can assumed to be at least 18 bytes and always consuming the wav
header gives the (possibly empty) codec private data.

Version 1 (Thanks to the Wayback Machine):
[1] http://web.archive.org/web/20120907004742/http://www.iis.net/community/files/media/smoothspecs/%5BMS-SMTH%5D.pdf

Version 2.1:
[2] http://download.microsoft.com/download/B/0/B/B0B199DB-41E6-400F-90CD-C350D0C14A53/%5BMS-SSTR%5D.pdf

Version 4:
[3] http://download.microsoft.com/download/9/5/E/95EF66AF-9026-4BB0-A41D-A4F81802D92C/%5BMS-SSTR%5D.pdf

The attached file contains three commits, the first two are preliminary changes, the third one is the actual fix for this issue. The commits can be imported with "git am" of course.

Ciao,
   Antonio

Review of attachment 258677 [details] [review]:

Generally looks good

::: ext/smoothstreaming/gstmssmanifest.c
@@ +588,3 @@
   gchar *codec_data_str =
       (gchar *) xmlGetProp (node, (xmlChar *) "CodecPrivateData");
   GstBuffer *codec_data = NULL;

Isn't this causing the < 18 byte codec_data to be used still? Shouldn't it instead really ignore it and not do anything with it?

My unde(In reply to comment #17)
> Review of attachment 258677 [details] [review]:
> 
> Generally looks good
> 
> ::: ext/smoothstreaming/gstmssmanifest.c
> @@ +588,3 @@
>    gchar *codec_data_str =
>        (gchar *) xmlGetProp (node, (xmlChar *) "CodecPrivateData");
>    GstBuffer *codec_data = NULL;
> 
> Isn't this causing the < 18 byte codec_data to be used still? Shouldn't it
> instead really ignore it and not do anything with it?

CodecPrivateData may not be present at all.

My understanding is that the WaveFormatEx attribute is used as an
_alternative_ to the CodecPrivateData one in older Manifest files.

So as I see it the rationale of the current code is:

  1. Check if "CodecPrivateData" is there, if so set codec_data:
     ...
     structure = gst_caps_get_structure (caps, 0);
     if (codec_data_str && strlen (codec_data_str)) {
       codec_data = gst_buffer_from_hex_string ((gchar *) codec_data_str);
     }
     ...

  2. if codec_data is still null try and see if there is a WaveFormatEx
     attribute.
     ...
     if (!codec_data) {
       GstMapInfo mapinfo;
       codec_data_str = (gchar *) xmlGetProp (node, (xmlChar *) "WaveFormatEx");
     ...

     [no leaks here, right? because if codec_data is still null then
     codec_data_str was null too]

I think proper handling for "CodecPrivateData" may still be needed to
support other streams, but my patch fixes the issue with WaveFormatEx
which is what this report is really about.

Thanks,
   Antonio

I agree with Sebastian, here.

In this case you are handling we know it is marked as a WaveFormatEx structure, so if it is smaller than 18 bytes it is better to ignore it as we don't know what exactly it could be.

OK, now I see what you are asking, I misinterpreted Sebastian when he was saying "not do anything with it", I thought it meant "do not parse it, and leave it up to the decoder"

Instead you mean that if the data is < 18 bytes we DROP it and don't pass any codec_data at all to the decoder.

It's OK by me, an update is coming soon.

Ping?

(In reply to comment #21)
> Ping?

Hey Thiago, I'll try to send the patch for tomorrow.

Created attachment 262904 [details] [review]
mssdemux: fix setting codec_data when using WaveFormatEx, version 2

New version attached.

Changes since the previous one:
  - ignore the WaveFormatEx content if it is less than 18 bytes of data.
  - declare GstMapInfo mapinfo in the block it is actually used.

Let me know if you liked a negative check better, like:
  if (!codec_data_str || codec_data_len < 18) {
    /* warning... */
  } else {
    /* common case */
  }

The attached file contains three commits, the first two are preliminary
changes, the third one is the actual fix for this issue. The commits can be
imported with "git am" of course.

Ciao,
   Antonio

Commits pushed:
8345c5acfbb7cf9bfcd356682c550a64d6348538
f02e7a24394541c0585dedb53325bb2b5cdf349f
c4fbff50dfa99e0f725a8690b0d0ff8509300a95

Thanks for the patches!