Can you make a capture of the stream? something like with:

  gst-launch udpsrc ! gdppay ! filesink location=capture.gdp

Then attach the captured file to this bug.

Created attachment 129395 [details]
FLV with nellymoser audio that segfaults

Works fine with current gst-ffmpeg master. Miguel, can you try with a git version ?

If not, could you specify extactly which distro and gstreamer package versions you're using ?

At the moment i am very busy and cannot compile gst-ffmpeg git.

I am using Ubuntu 8.10 with this GStreamer versions compiled by myself:
- GStreamer (gstreamer-0.10.22)
- GStreamer plugins base (gst-plugins-base-0.10.22)
- GStreamer plugins good (gst-plugins-good-0.10.12)
- GStreamer plugins bad (gst-plugins-bad-0.10.10)
- GStreamer plugins ugly (gst-plugins-ugly-0.10.10)
- GStreamer ffmpeg (gst-ffmpeg-0.10.6)

I downloaded gst-ffmpeg from git today, compiled it, and here is the backtrace:

+ Trace 213200

Reproducible (using gst-launch, rather than some weirdo java thingy). 32 bit linux here.


Program received signal SIGSEGV, Segmentation fault.

+ Trace 213201

The buffer allocated by gstreamer (for ffmpeg to decode into) is 8-byte aligned.

ffmpeg (using sse2 code) requires a 16-byte aligned buffer in this case.

It's misaligned, so you get a crash.

gstffmpeg could use a temporary, 16-byte-aligned, buffer, then copy into the actual destination GstBuffer (this requirement is documented in avcodec.h, sort of, in hand-wavey language).

This would be the info from avcodec.h for avcodec_decode_audio2

 * @note You might have to align the input buffer \p buf and output buffer \p
 * samples. The alignment requirements depend on the CPU: On some CPUs it isn't
 * necessary at all, on others it won't work at all if not aligned and on others
 * it will work but it will have an impact on performance. In practice, the
 * bitstream should have 4 byte alignment at minimum and all sample data should
 * be 16 byte aligned unless the CPU doesn't need it (AltiVec and SSE do). If
 * the linesize is not a multiple of 16 then there's no sense in aligning the
 * start of the buffer to 16.
 *

Will cook up a patch for the pre-release.

Created attachment 130186 [details] [review]
Use av_malloc() to allocate memory.

av_malloc() takes care or memory alignment... so let's just use it !

Will try it on a 32bit machine later on if nobody has commented on this before.

Comment on attachment 130186 [details] [review]
Use av_malloc() to allocate memory.

>-  /* outgoing buffer */
>-  *outbuf = gst_buffer_new_and_alloc (AVCODEC_MAX_AUDIO_FRAME_SIZE);

Oh wow, I would have assumed g_malloc() provides a reasonably aligned return value. Maybe we should fix gst_buffer_new_and_alloc() to do this automatically? Or provide new functions for that? (Both doesn't help us now of course)

>+  /* outgoing buffer. We use av_malloc() to have properly aligned memory. */
>+  *outbuf = gst_buffer_new ();
>+  GST_BUFFER_DATA (*outbuf) = GST_BUFFER_MALLOCDATA (*outbuf) = av_malloc(AVCODEC_MAX_AUDIO_FRAME_SIZE);
>+  GST_BUFFER_SIZE (*outbuf) = AVCODEC_MAX_AUDIO_FRAME_SIZE;

Don't we also need an GST_BUFFER_FREE_FUNC (*outbuf) = av_free; ?

What about the input buffer? Not sure how to read the docs, but it seems like the input buffer needs to be at least 4-byte aligned?

(In reply to comment #10)
> (From update of attachment 130186 [details] [review] [edit])
> >-  /* outgoing buffer */
> >-  *outbuf = gst_buffer_new_and_alloc (AVCODEC_MAX_AUDIO_FRAME_SIZE);
> 
> Oh wow, I would have assumed g_malloc() provides a reasonably aligned return
> value.

  Nope, it only provides (arch-size) bytes alignment (i.e. on 32bit : 4 bytes, and on 64bits: 8 bytes). The ffmpeg implementation does 128bit (16bytes) alignment (by using memalign/posix_memalign).

> Maybe we should fix gst_buffer_new_and_alloc() to do this automatically?
> Or provide new functions for that? (Both doesn't help us now of course)

  Was thinking about that indeed. Considering that any sse/sse2 code (and maybe other technologies) require aligned memory... it would make much more sense.
  New functions would make more sense.

> 
> >+  /* outgoing buffer. We use av_malloc() to have properly aligned memory. */
> >+  *outbuf = gst_buffer_new ();
> >+  GST_BUFFER_DATA (*outbuf) = GST_BUFFER_MALLOCDATA (*outbuf) = av_malloc(AVCODEC_MAX_AUDIO_FRAME_SIZE);
> >+  GST_BUFFER_SIZE (*outbuf) = AVCODEC_MAX_AUDIO_FRAME_SIZE;
> 
> Don't we also need an GST_BUFFER_FREE_FUNC (*outbuf) = av_free; ?

  We could for safety, although it will not be doing anything fancy (apart from calling free) if we have CONFIG_MEMALIGN_HACK deactivated (which is the case in our supported builds).

> 
> What about the input buffer? Not sure how to read the docs, but it seems like
> the input buffer needs to be at least 4-byte aligned?
> 
  Indeed. I guess we could check if the input buffer is 32bit aligned, and if not, create a temporary buffer with av_malloc(), pass that to the decoding function, and then free it.

  Will create an updated patch for this and test it.

commit 98167578c45b91b5b73f4e1165fb68d03b10ed92
Author: Edward Hervey <bilboed@bilboed.com>
Date:   Fri Mar 6 17:37:51 2009 +0100

    ffmpegdec: Make sure we provide 16 byte aligned data to ffmpeg. Fixes #572863
    
    We simply allocate the memory using ffmpeg's av_malloc which provides us
    with properly memalign'ed data.
    This avoids write-outside-of-bounds when sse/altivec code is being used.

diff --git a/ext/ffmpeg/gstffmpegdec.c b/ext/ffmpeg/gstffmpegdec.c
index aeec910..019140c 100644
--- a/ext/ffmpeg/gstffmpegdec.c
+++ b/ext/ffmpeg/gstffmpegdec.c
@@ -1794,8 +1794,12 @@ gst_ffmpegdec_audio_frame (GstFFMpegDec * ffmpegdec,
       GST_TIME_ARGS (in_timestamp), GST_TIME_ARGS (in_duration),
       GST_TIME_ARGS (ffmpegdec->next_ts));
 
-  /* outgoing buffer */
-  *outbuf = gst_buffer_new_and_alloc (AVCODEC_MAX_AUDIO_FRAME_SIZE);
+  /* outgoing buffer. We use av_malloc() to have properly aligned memory. */
+  *outbuf = gst_buffer_new ();
+  GST_BUFFER_DATA (*outbuf) = GST_BUFFER_MALLOCDATA (*outbuf) =
+      av_malloc (AVCODEC_MAX_AUDIO_FRAME_SIZE);
+  GST_BUFFER_SIZE (*outbuf) = AVCODEC_MAX_AUDIO_FRAME_SIZE;
+  GST_BUFFER_FREE_FUNC (*outbuf) = av_free;
 
   len = avcodec_decode_audio2 (ffmpegdec->context,
       (int16_t *) GST_BUFFER_DATA (*outbuf), &have_data, data, size);

Unfortunately that only fixes the audio case. I can reproduce the same issue on my 32 powerpc with just using ffdec_mpeg4. A quick hack to alloc_output_buffer reallocate its own buffer with av_malloc when gst_pad_alloc_buffer_and_set_caps returns a buffer not aligned to 16 bytes fixes the issue... But that doesn't seem like the proper way of fixing it

Alas... doing what you suggest is indeed the proper fix.

If the downstream element properly allocates 128bit-aligned then you won't see a difference.

Changing title of the bug according, will cook up a patch.

Created attachment 130269 [details] [review]
Make sure we provide 128bit-aligned buffers to ffmpeg.

    Make sure we provide ffmpeg with 128bit-aligned data.
    
    Add a new function new_aligned_buffer() which creates a GstBuffer of
    the requested size/caps, with the memory being allocated/freed by ffmpeg's
    av_malloc/av_free which guarantees properly aligned memory.
    Added a can_allocate_aligned internal property which we use to figure out
    whether downstream can provide us with 128bit aligned buffers.

(In reply to comment #15)
> Created an attachment (id=130269) [edit]
> Make sure we provide 128bit-aligned buffers to ffmpeg.

That fixes the issue on my ppc indeed, thanks

I have actually a fixme for memalign() in my local code for quite some time, but wonder how portable this is. On macOS its probably fine. Anyone know the story on windows.
128bit aligned would help e.g. with DSP based environments too. Too bad that there is no guarrantee for that because of subbuffers :/

Forgot to close bug again after committing patch.