Created attachment 289561 [details]
backtrace log of omxaacdec that works well

Please refer to each ref_count of 'allocatorsysmem0' display prints. It doesn't stay constantly but eventually increases forever in the case of amr.

In omxamrdec, I found that the number of GstMemory created in gst_base_parse_scan_frame(), between two gst_omx_audio_dec_loop() is about 80 however, each gst_omx_audio_dec_loop() frees only about 10 of them in its gst_audio_decoder_finish_frame(). Thus, it means that the pipeline gets 70 of GstMemory when it performs a gst_omx_audio_dec_loop().

How possibly does it happen??

Why gst_omx_audio_dec_loop() in omxamrdec isn't repeated properly when the pipeline stores 3~4 GstMemories as that of omxaacdec does?

That could mean that your hardware codec consumes many input buffers and then only outputs less output buffers with the same data. When calling gst_audio_decoder_finish_frame() you will have to provide the base class with the number of raw audio samples that are inside the output buffer, so that the base class can properly remove the data from the adapter.

For this see the spf calculation that I added for MP3, and where we set the spf to 1152 or whatever depending on the input caps... and then based on that finish a specific number of samples.

Thanks. You seems right. I had given it just 1024, the number for AAC and confirmed that the memory leak disappeared with 576, it still has an issue that it dies before completes the entire play, though.

One question, how did you know that AAC needs 1024 or that MP3 does 576(1152)? while it seems like depending on each H/W decoder spec..

Slomo, sorry for the confusion. On my second test, it still has a huge leak and what I found are

1. spf 1024, spf 576 gives 8 frames to gst_audio_decoder_finish_frame() following the frame calculation if gst_omx_audio_dec_loop(). With this 8 frames, the difference of the refcount of sysallocatormem between two gst_omx_audio_dec_loop() is about 50. Moreover, there was a sound loss. Now I recognized that the loss may be because it drops 8 frames.

2. with spf -1, no loss and the difference of the refcount is about 40.


I'm still working on it.. thank you for the support.

With AMR you have 50 frames per second, so depending on the sample rate (8000 for NB, 16000 for WB) this would be 8000/50=160 or 16000/50=320 samples per frame.

This is not dependant on the hardware codec, but the codec itself. It might only be that your hardware codec outputs multiple frames per buffer, which is what the code should handle fine already.

Using -1 for the number of frames to finish is almost always wrong.


Does it work with the above numbers?

Yes, the 160 fixed the problem. Thanks.

Created attachment 290128 [details] [review]
confirmed twice that this patch works

Review of attachment 290128 [details] [review]:

Thanks for the patch, looks good :) Just some minor comments

::: omx/gstomxamrdec.c
@@ +1,2 @@
+/*
+ * Copyright (C) 2014, Sebastian Dröge <sebastian@centricular.com>

You probably want to add your or your company's name here as a second line :)

@@ +64,3 @@
+  audiodec_class->cdata.default_sink_template_caps = "audio/AMR, "
+      "rate=(int)[8000,48000], "
+      "channels=(int)[1,9]";

It's audio/AMR for narrow-band and audio/AMR-WB for wide-band. The narrow-band only supports 8000Hz, wide-band only supports 16000Hz

@@ +79,3 @@
+{
+  /* FIXME: Other values exist too! */
+  self->spf = -1;

Remove the FIXME comment here, it is wrong

@@ +131,3 @@
+
+  amr_param.nChannels = channels;
+  amr_param.nBitRate = rate;       /* unknown */

Not really unknown, isn't it? :)

@@ +197,3 @@
+static gboolean
+gst_omx_amr_dec_get_channel_positions (GstOMXAudioDec * dec,
+    GstOMXPort * port, GstAudioChannelPosition position[OMX_AUDIO_MAXCHANNELS])

AMR only supports mono, so add assertions (g_return_val_if_fail (pcm_param.nChannels == 1, GST_AUDIO_CHANNEL_POSITION_MONO))  for all the other cases here and always return MONO

Review of attachment 290128 [details] [review]:

::: omx/gstomxamrdec.c
@@ +95,3 @@
+
+  gst_omx_port_get_port_definition (port, &port_def);
+  port_def.format.audio.eEncoding = OMX_AUDIO_CodingAMR;

Does it need different settings for AMR vs. AMR-WB? There's e.g. OMX_AUDIO_AMRBandModeNB0 and OMX_AUDIO_AMRBandModeWB0

slomo, thank you for the correction.

--
AMR only supports mono, so add assertions (g_return_val_if_fail
(pcm_param.nChannels == 1, GST_AUDIO_CHANNEL_POSITION_MONO))  for all the other
cases here and always return MONO
--
Would you mean 
g_return_val_if_fail (pcm_param.nChannels == 1, FALSE)?

as the function gst_omx_amr_dec_get_channel_positions requires gboolean type return?

BTW, if AMR only supports mono, why don't we specify its channels of its cap to only channels=(int)1?

(In reply to comment #12)
> slomo, thank you for the correction.
> 
> --
> AMR only supports mono, so add assertions (g_return_val_if_fail
> (pcm_param.nChannels == 1, GST_AUDIO_CHANNEL_POSITION_MONO))  for all the other
> cases here and always return MONO
> --
> Would you mean 
> g_return_val_if_fail (pcm_param.nChannels == 1, FALSE)?
> 
> as the function gst_omx_amr_dec_get_channel_positions requires gboolean type
> return?

Yes

> BTW, if AMR only supports mono, why don't we specify its channels of its cap to
> only channels=(int)1?

That too, I missed that. Sorry :) Nonetheless catching this kind of error inside the code as mentioned above is also useful.

Created attachment 290394 [details] [review]
modified as suggested

Hi slomo, the patch was fixed and uploaded again. Please just let me know whatever feels wrong. Thanks.

Accidentally uploaded the file with my personal mail but it was me. ;)
(I can't find a delete option)

Review of attachment 290394 [details] [review]:

::: omx/gstomxamrdec.c
@@ +64,3 @@
+
+  audiodec_class->cdata.default_sink_template_caps = "audio/AMR, "
+      "rate=(int)[8000,48000], "

It can only be 8000 for audio/AMR, and 16000 for audio/AMR-WB. So make these caps:
"audio/AMR,rate=(int)8000,channels=(int)1; audio/AMR-WB,rate=(int)16000,channels=(int)1"

@@ +132,3 @@
+
+  amr_param.nChannels = channels;
+  amr_param.nBitRate = rate;       /* unknown */

Not really unknown, is it? :) But the docs say that bitrate is a read-only field, and also does not contain the sample rate.

I think you need to set eAMRBandMode properly though. See the possible values of the OMX_AUDIO_AMRBANDMODETYPE enum

@@ +184,3 @@
+    return TRUE;
+
+  if (amr_param.nBitRate != rate)

Same here, bitrate is not sample rate.

I fixed the caps and removed the lines you mentioned above for nBitRate. It works. :) However, currently I can't figure out the eAMRBandMode as actually my qcom board doesn't support WB while it's just find with eAMRBandMode=0 for NB.

So, can we just leave it to 0 and add some comment like '/*FIXME: this may need to be a specific value'?

Thanks
Jun

Yes, sounds good

Created attachment 290569 [details] [review]
a refined patch

slomo, please let me check its integrity one more. I found that suddenly omxaacenc isn't working after this patch. Probably some mistake not that hard to fix. :) I'll add a comment as soon as I figure out.

It's weird that omxaacenc somehow once printed out 'Unsupported format:0'.. but as soon as I reflash the (exactly same) libgstomx.so to the device, it's gone. :-( hopefully, it was a device issue..

Anyway I just tested 10 times and confirmed that the patch works.

Please run gst-indent over your code in the future, and also fix all compiler warnings you get :) I did that now and integrated those changes into your commit.

commit 8936f6634c878973d966df53d23505a7c05e0f0c
Author: Jun Ji <jun.ji@lge.com>
Date:   Thu Nov 13 09:55:02 2014 +0900

    omx: Add omxamrdec
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739333

Thank!! OK, I'll do for the next ones!