Bad audio meta

[heftig]

Current master (79fbffc) makes our app explode:

GStreamer-Audio: gst_audio_buffer_map: assertion 'meta->samples <=
  gst_buffer_get_size (gstbuffer) / GST_AUDIO_INFO_BPF (&meta->info)' failed

I think I can spot some issues:

• audio_info isn't updated when the caps change.
• The buffer size is passed to gst_buffer_add_audio_meta instead of the sample count.

[MathieuDuponchelle]

While 1. is possible in theory, I don't think it can happen with cef (though it can't hurt to check that). 2. is indeed just wrong afaict, @philn any reason for just doing:

gst_buffer_add_audio_meta (*buffer, &push_data->demux->audio_info, gst_buffer_get_size (*buffer), NULL);

I reckon here we want gst_buffer_get_size (*buffer) / push_data->demux->audio_info.bpf * push_data->demux->audio_info.channels , can you try that @heftig ?

[MathieuDuponchelle]

or gst_buffer_get_size (*buffer) / GST_AUDIO_INFO_BPS (&push_data->demux->audio_info), should be equivalent

[philn]

omg what was I thinking... yeah this code i wrote is clearly wrong.

[MathieuDuponchelle]

omg what was I thinking... yeah this code i wrote is clearly wrong.

heh, happens, I could have caught it when reviewing too :)

[philn]

I guess this can be closed then! Next time a "Fixes #issuenumber" in commit message should do it automatically. Thanks @SteveMcFarlin !

[heftig]

This is still broken as far as I can tell. It should be using BPF and not BPS.

gst_buffer_add_audio_meta expects the number of samples in the buffer. The wording is a bit ambiguous, but the example given for non-interleaved layout makes it clear that the buffer size should be equal to channels * samples * sample_stride.

BPS produces the bytes per sample (the sample_stride), while BPF produces the bytes per frame (channels * sample_stride).

So in the case of a non-mono stream I'd expect the meta to still be broken.

[MathieuDuponchelle]

I don't understand your logic? If gst_buffer_add_audio_meta() expects a number of samples, then why do you want to calculate and pass a number of frames? Looking at the way nsamples is computed in gst_ffmpegauddec_audio_frame for instance, it is completely clear to me that gst_buffer_add_audio_meta() does not expect a number of frames.

Are you actually encountering an issue here?

[MathieuDuponchelle]

(@heftig ^)

[heftig]

In the context of audio-info, a frame is defined as one sample for each channel. Now look at the way the way gst_ffmpegauddec_audio_frame adds the meta:

    nsamples = ffmpegdec->frame->nb_samples;

/* ... */

    /* ffmpegdec->frame->linesize[0] might contain padding, allocate only what's needed */
    output_size = nsamples * byte_per_sample * channels;

    *outbuf =
        gst_audio_decoder_allocate_output_buffer (GST_AUDIO_DECODER
        (ffmpegdec), output_size);

/* ... */

      meta = gst_buffer_add_audio_meta (*outbuf, &ffmpegdec->info, nsamples,
          NULL);

And the definition of nb_samples:

    /**
     * number of audio samples (per channel) described by this frame
     */
    int nb_samples;

frame here means an FFmpeg frame, equivalent to a GStreamer buffer, not an audio-info frame. So nsamples is not the total amount of samples, it is the amount of samples in each channel in this buffer, i.e. the amount of audio-info frames.

[MathieuDuponchelle]

OK, this API is very poorly documented then :) I see that:

subprojects/gst-plugins-base/tests/check/elements/audioconvert.c:  gst_buffer_add_audio_meta (inbuffer, &in_info, inlength / in_info.bpf, NULL);

Which also confirms what you're saying, can you prepare a patch? A patch against the documentation of gst_buffer_add_audio_meta would also seem in order if you feel like it?