Created attachment 343421 [details] [review]
Fix samples per buffer calculation

Review of attachment 343421 [details] [review]:

::: omx/gstomxaacenc.c
@@ -505,3 @@
 {
-  /* FIXME: Depends on the profile at least */
-  return 1024;

This should return the number of samples per encoded audio frame, no? If it was about the raw data, then the base class can as well calculate it by itself.

Also, could you please elaborate on what this fixes exactly in what situation etc? :)

OK, I see I confused in/out buffers.

Before this patch, I was getting an assert in the base class about wrong number of samples passed, and I found this placeholder. I guess the patch fixed the assert by chance. I have to build/reflash another setup before I can reproduce and give the exact assert I had, then work out how to actually fix that.

I can't reproduce, but I also can't recall what test I was running, as the bash history gets wiped with every flash and the terminal scrollback's gone.

Anyway, the error I was getting is:

    GST_ELEMENT_ERROR (enc, STREAM, ENCODE,
        ("received more encoded samples %d than provided %d as inputs",
            samples, priv->offset / ctx->info.bpf), (NULL));

from gst-libs/gst/audio/gstaudioencoder.c

I have changed the patch to the sample numbers mentioned in https://wiki.multimedia.cx/index.php?title=Understanding_AAC now.

Created attachment 343436 [details] [review]
omxaacenc: Fix samples per buffer calculation

Comment on attachment 343436 [details] [review]
omxaacenc: Fix samples per buffer calculation

>@@ -393,6 +393,8 @@ gst_omx_aac_enc_get_caps (GstOMXAudioEnc * enc, GstOMXPort * port,
>   gint mpegversion = 4;
>   const gchar *stream_format = NULL, *profile = NULL;
> 
>+  ((GstOMXAACEnc *)enc)->samples_per_frame = 1024;

>@@ -493,6 +495,8 @@ gst_omx_aac_enc_get_caps (GstOMXAudioEnc * enc, GstOMXPort * port,
>     GST_DEBUG_OBJECT (enc, "setting new codec_data");
>     gst_caps_set_simple (caps, "codec_data", GST_TYPE_BUFFER, codec_data, NULL);
> 
>+    ((GstOMXAACEnc *)enc)->samples_per_frame = map.data[1] & 4 ? 960 : 1024;
>+

Please add a local variable for this instead of doing the cast inline, twice :)


>+    ((GstOMXAACEnc *)enc)->samples_per_frame = map.data[1] & 4 ? 960 : 1024;

I think this assumes that no 'unusual' sample rate is used (otherwise there's an extra 24 bit or so for the sample rate). I don't see a restriction on input sample rates in the caps anywhere, but only had a quick look.

Might also be worth checking if the encoder actually outputs one AAC frame per output buffer.

The extra data is built just the few lines beforehand, so will not have an extra 24 bits (though I think you're right it might just bork if one is passed).

About not-one-frame per buffer... I guess it depends on the hardware encoder :/

> The extra data is built just the few lines beforehand, so will not have an
> extra 24 bits (though I think you're right it might just bork if one is
> passed).

Ah yes, I missed that, sorry :) I assumed it came from the encoder.

In that case I'm not sure if the extra code makes sense though - don't we know the flag will always be unset then? Is there anything in the omx aac struct that maps to this?


> About not-one-frame per buffer... I guess it depends on the hardware encoder
> :/

Yeah, I was more suggesting to check this for debugging since I'm not sure if this patch is really what's going to fix your original problem (does it?)

Well, given I can't reproduce my problem now...
I can close this bug I suppose :)