Created attachment 197043 [details]
file to reproduce the issue

Created attachment 197135 [details] [review]
multipartdemux: guard against having no MIME type

The code would previously crash trying to insert a NULL string
into a hash table.
It does seem a little broken that indexing is done by MIME type
and not by index though, unless the spec says there cannot be
two parts with the same MIME type.

thanks, now exit without segfault: here is the error:

Boundary not found in the multipart header

however wireshark show this:

HTTP/1.1 200 OK
Content-Type: multipart/mixed;boundary=myboundary

--myboundary
Content-Length: 3868

so the boundary is there

what is missing is the Content-Type, for example here is a stream that multipart can handle:

HTTP/1.1 200 OK
Expires: Mon, 01 Jul 1980 00:00:00 GMT
Cache-Control: no-cache, no-store, must-revalidate
Pragma: no-cache
Content-Type: multipart/x-mixed-replace;boundary=myboundary

--myboundary
Content-Type: image/jpeg
Content-Length: 33575

... data ...

and this is the one that doens't work

HTTP/1.1 200 OK
Content-Type: multipart/mixed;boundary=myboundary

--myboundary
Content-Length: 3868

..data...

maybe multipartdemux should demux the data and forward if no content type is given

Hi Vincent,

I used another approach to handle this case. For now if mime is null I'm hardcoding the mime for my audio source if mime is null. I think it is easy enough to add a property to multipartdemux such as force-mime-if-null that allow to externally set the mime if no mime is found. If you think this property could be included in -good I'll find some time to make it, if not I'll use my modified hardcoded multipartdemux (it not break multipartdemux since I'm hardconding pad caps only if mime is null),

thanks
Nicola

Created attachment 197244 [details] [review]
attached a patch that handle the null mime type and add two more properties

This patch add the following properties:

- force-mime-if-null, allow to specify a mime if not present in the stream
- skip-first-n-bytes, remove the first n bytes from every frame, it is needed to handle some custom format

This patch allow to receive both audio and video from trendnet network cameras

Hi, I asked to trendnet if they have problems to make public available the code that allow to use their cameras in gstreamer, here is my mail:

"Hi,

I did some modifications to the gstreamer open source multimedia framework (http://gstreamer.freedesktop.org/) that make it able to receive both audio and non standard mjpeg (and I think mpeg4 too, it is not supported by my camera so I not tested it) from TV-IP121WM, do you have problems if I make this code publicy available in gstreamer (LGPL licensed)?"

here is the answer from trendnet:

"9/27/2011 2:35:52 PM
Johnny Hernandez
(Technical Support Rep)	Dear customer,

There would be no problem making code public."

That does not fix the issue of indexing by MIME type, does it ?
If more than one part has the same MIME type, it won't work IIRC (not that I know if it is allowed or not by the multipart format).
Indexing by, er, index, would solve both issues (your camera not giving a MIME type, and cases with more than one part having the same MIME type).

(from memory, I haven't looked at the code again).

Hi Vincent,

I think the patch solve both the problems, using the attached test file I have the following (with the patch):

gst-launch filesrc location=/tmp/test.multipart ! multipartdemux ! decodebin2 ! pulsesink
Impostazione della pipeline a PAUSED ...
La pipeline è in PREROLLING ...
ERRORE: dall'elemento /GstPipeline:pipeline0/GstMultipartDemux:multipartdemux0: Impossibile de-multiplare lo stream.
Informazioni di debug aggiuntive:
multipartdemux.c(519): multipart_parse_header (): /GstPipeline:pipeline0/GstMultipartDemux:multipartdemux0:
Boundary not found in the multipart header
ERRORE: la pipeline non vuole fare il preroll.
Impostazione della pipeline a NULL ...
Esecuzione di free sulla pipeline...

so it give errors if no mime is found,

the patch allow to specify a mime as a property and optionally to skip the first n bytes of the buffer (it is quite common in custom format add some bytes to the begin of a frame/buffer), so if you apply the patch you can use a pipeline such this to play the attached file:

gst-launch filesrc location=/tmp/test.multipart ! multipartdemux skip-first-n-bytes=28 force-mime-if-null="audio/x-raw-int,rate=8000,channels=1,endianness=1234,width=16,depth=16,signed=true"  ! decodebin2 ! pulsesink

the relevant code in the patch is this:

     return mppad;
+  } else {
+    GST_WARNING_OBJECT (demux,
+        "No MIME type e force-mime-if-null not setted, cannot create pad");
+    if (created) {
+      *created = FALSE;
+    }
+    return NULL;
   }

I'm not using anymore the trendnet camera, I think the patch could be useful in general,

Nicola

Sure, but my point was not that. It was:
If you have one part with MIME type (eg) text/plain
If you have another part with same MIME type text/plain

Then a single pad will be created, instead of one per part, because the indexing is done by a hash table keyed on the MIME type, so there is a collision.
Though maybe it was intended to be that way, I tried creating such a file, and buffers flow onto the same pad, so...

is this handled with the actual multipartdemux without my patch? Is this a real case? 

However you are right if you use force-mime-if-null property and you don't have Content-Type setted all the part without Content-Type will be forced to the value you give using force-mime-if-null

It's not handled by the current code, no. That's what I was saying when I posted my original patch. Changing it to not do that (for instance, indexing each new part by their order of appearance) would fix the bug without having to add a "default" MIME type (since that MIME type would not be used as a key).

And I don't know if it's a real case, that's what I meant by "unless the spec says there cannot be two parts with the same MIME type".

Review of attachment 197244 [details] [review]:

I'm not big fan of that approach, and can't find anything in the spec to justify. I think the input is invalid, we should not crash of course.

::: gst/multipart/multipartdemux.c
@@ +202,3 @@
+          DEFAULT_MIME_IF_NULL,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS));
+

I don't like these property names. Shorter names would do just fine. Also, rules is 80 char/lines.

Review of attachment 197135 [details] [review]:

Minor comment, feel free to merge when/if you are happy with it. Content-type seems mandatory, so skipping or failing seems right. I didn't found anything in RFC 2046 that would allow having two streams with the same content type. This means it would have to be implement with a GStreamer extension, e.g. X-gstreamer-stream-id: <stream-id>. We could do that eventually if really needed.

::: gst/multipart/multipartdemux.c
@@ +349,3 @@
+  } else {
+    GST_WARNING_OBJECT (demux, "No MIME type, cannot create pad");
+    return NULL;

Maybe I miss something, or code have changed, but it seems like NULL isn't handled have calling gst_multipart_find_pad_by_mime(). At the same time, it will never be called with the following fix. So maybe we should put g_return_val_if_fail(), and do that at both place, as a crash avoidance, rather then handling it and crashing later ?

@@ +590,3 @@
+    } else if (G_UNLIKELY (!multipart->mime_type)) {
+      GST_DEBUG_OBJECT (multipart, "content has no MIME type.");
+      gst_adapter_flush (adapter, size - datalen);

That looks good.

Yes, 7d78a68c introduced unconditional use of the pad after call of this function. Should be easy to patch too.

As for the multiple entries with the same content type, you did not find anything that allowed it, but did you find anything that forbids it ? It would be surprising if this was disallowed. After all, this is how cameras do their JPEGish streaming, lots of little parts with the same content type.

> Yes, 7d78a68c introduced unconditional use of the pad after call of this
> function.

It did? As far as I can tell it adds some code that uses srcpad->foo before the already-existing gst_pad_push(srcpad->pad) call. I don't think this counts as 'introduced unconditional use'?

I removed the test in the function as the test in the caller shorts it out indeed.


commit 7e278e6b22fe2eea8b620996d6f3c7bbbb30c119
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Jun 11 17:43:42 2014 +0100

    multipartdemux: guard against having no MIME type
    
    The code would previously crash trying to insert a NULL string
    into a hash table.
    It does seem a little broken that indexing is done by MIME type
    and not by index though, unless the spec says there cannot be
    two parts with the same MIME type.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=659573

(In reply to comment #18)
> > Yes, 7d78a68c introduced unconditional use of the pad after call of this
> > function.
> 
> It did? As far as I can tell it adds some code that uses srcpad->foo before the
> already-existing gst_pad_push(srcpad->pad) call. I don't think this counts as
> 'introduced unconditional use'?

Maybe there already was such use then ? I did not checkout that rev to look further in time. In any case, as Nicolas pointed out, that branch was not taken when there was no MIME type, so that explains why it would not have crashed with any previous dereference, so all is good (and I removed that part of my patch, since it's pointless anyway).

(In reply to comment #17)
> As for the multiple entries with the same content type, you did not find
> anything that allowed it, but did you find anything that forbids it ? It would
> be surprising if this was disallowed. After all, this is how cameras do their
> JPEGish streaming, lots of little parts with the same content type.

I'm still trying to understand the use case you have in mind. The camera I'm dealing with have 1 stream (mime) for jpeg data, and 1 stream (mime) for audio data. Never seem any camera with multiple video streams. Note that this is all a bit of an abuse of the format, after all this was designed to attach stuff to emails, where each part is simply a different file.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/49.