After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 661624 - flvmux: overstates the number of metadata elements when 'streamable=true'
flvmux: overstates the number of metadata elements when 'streamable=true'
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-10-13 04:33 UTC by George Chriss
Modified: 2015-09-05 13:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to crtmpserver to fix parsing of FLV metadata (1.47 KB, patch)
2011-10-25 15:22 UTC, Vincent Penquerc'h
rejected Details | Review
One-line removal of tags_written++; (416 bytes, patch)
2013-10-11 15:13 UTC, George Chriss
committed Details | Review

Description George Chriss 2011-10-13 04:33:48 UTC
When sending a H.264 stream to crtmpserver the flvmux element appears to overstate the number of metadata elements sent if given the 'streamable=true' parameter, thus generating an exception with crtmpserver's AFM0 deserializer.  

Two test runs, the first without 'streamable=true' and the second with:
 http://pastebin.com/bYTyrbb9

Removing one of the 'tags_written++;' lines results in expected behavior:
./gst/flv/gstflvmux.c
  tmp = gst_buffer_new_and_alloc (2 + 0 + 1);
  data = GST_BUFFER_DATA (tmp);
  data[0] = 0;                  /* 0 byte size */
  data[1] = 0;
  data[2] = 9;                  /* end marker */
  script_tag = gst_buffer_join (script_tag, tmp);
-  tags_written++;

But this is just a workaround and I'm not very good with Action Message Format (AMF0).
Comment 1 Jan Schmidt 2011-10-16 13:20:24 UTC
This might be related to is the tag merging mode that changes based on streamable=true/false
Comment 2 Vincent Penquerc'h 2011-10-25 15:22:02 UTC
Created attachment 199939 [details] [review]
patch to crtmpserver to fix parsing of FLV metadata
Comment 3 Vincent Penquerc'h 2011-10-25 15:22:37 UTC
Seems like it's a parsing issue in crtmpserver.
Patch sent upstream, and attached here for convenience.
Comment 4 C++ RTMP Server 2011-10-26 12:34:45 UTC
Thank your for the time invested into this issue and for the patch. However, I don't agree with you. According to amf0_spec_121207.pdf, here is the definition of ECMA Array:

------------begin copy-paste-------------
An ECMA Array or 'associative' Array is used when an ActionScript Array contains non- ordinal indices. This type is considered a complex type and thus reoccurring instances can be sent by reference. All indices, ordinal or otherwise, are treated as string 'keys' instead of integers. For the purposes of serialization this type is very similar to an anonymous Object.

associative-count = U32
ecma-array-type = associative-count *(object-property)
------------end copy-paste-------------

So, it is what it is: we must have exactly that number of elements, not more, not less.

I know that those 2 pieces of documentation collide, but the code responsible for parsing AMF0 in normal conditions (RTMP connections) is also responsible for parsing arbitrary AMF0 data from various sources (including FLV files). I also believe that is the case with many other RTMP projects.

Best regards,
Andrei
Comment 5 Vincent Penquerc'h 2011-10-26 14:31:13 UTC
Then I guess the patch in the original report makes sense. E.4.4.4 from http://download.macromedia.com/f4v/video_file_format_spec_v10_1.pdf shows an end marker after the elements, but states the element count is only approximate, so I guess getting the count exactly right excluding the end marker is as right as anything else, unless there's some more precise spec somewhere else.
Comment 6 Sebastian Dröge (slomo) 2013-08-21 19:03:48 UTC
Is this still a problem and does someone have an updated patch?
Comment 7 George Chriss 2013-10-11 04:04:13 UTC
Confirming the issue is still present using the latest checkouts of GStreamer (1.x series) + crtmpserver (no recent commits).  The workaround patch in the original report still works.
Comment 8 Nicolas Dufresne (ndufresne) 2013-10-11 13:54:18 UTC
(In reply to comment #7)
> Confirming the issue is still present using the latest checkouts of GStreamer
> (1.x series) + crtmpserver (no recent commits).  The workaround patch in the
> original report still works.

Would it be possible to attach a correct patch (git format-patch) and this is probably worth a unit test and comment pointing out to the spec.
Comment 9 George Chriss 2013-10-11 15:13:02 UTC
Created attachment 257008 [details] [review]
One-line removal of tags_written++;
Comment 10 Sebastian Dröge (slomo) 2014-05-03 07:46:41 UTC
Vincent, how should we proceed with this? I think it's ok to just not count the end marker, but also crtmpserver has to be fixed to not expect things that are not in the spec.
Comment 11 Vincent Penquerc'h 2014-05-05 09:49:46 UTC
Well, if I remember correctly:

- the patch from George Chriss works
- there are two different rules in different specs

So I'd vote for applying that patch. And hope it does not breaks some other software.
Comment 12 Jan Schmidt 2015-09-02 16:57:45 UTC
I'm going to apply this after 1.6 is released. It seems to work with at least one other RTMP server fine (Microsoft Azure), and seems reasonable according to either of the specs linked above.
Comment 13 Nicolas Dufresne (ndufresne) 2015-09-02 17:42:59 UTC
Review of attachment 257008 [details] [review]:

Two votes ;-P
Comment 14 Jan Schmidt 2015-09-05 13:46:43 UTC
That'll do. Pushed.