opus icon indicating copy to clipboard operation
opus copied to clipboard

v1.3.1 -- Memory scribble when changing ENCODER_NUM_CHANNELS from 2 -> 1

Open judgeh opened this issue 5 years ago • 3 comments

I am using opus, tag v1.3.1, with ENCODER_NUM_CHANNELS set to 1 (default=2) to reduce the encoder memory requirements.

With this configuration, line 166 in enc_API.c causes a memory scribble as the encoder ctx does not have the second channel allocated:

psEnc->state_Fxx[ 0 ].sCmn.nFramesEncoded = psEnc->state_Fxx[ 1 ].sCmn.nFramesEncoded = 0;

This was overwriting a pointer in an unrelated part of my application, resulting in undefined behavior. I believe this is a critical bug. I have addressed it in my application so this is not critical for me, but just wanted to notify you guys.

judgeh avatar Oct 07 '20 16:10 judgeh

I am not sure it was ever intended or officially tested for this use. How much have you managed to reduce the memory requirement and does it enable it to run on lower end devices or what is the main purpose?

xnorpx avatar Oct 19 '20 15:10 xnorpx

Ah, I assumed from the comments that it was supported:

/* Max number of encoder channels (1/2) */ #define ENCODER_NUM_CHANNELS 2

But yes, we are running opus on a chip with very little memory. As far as I can tell, we have reduced the memory footprint with no side effects other than this problem. However, now that you mention it hasn't been tested, I wonder if there are side effects that we aren't noticing right now.

Any particular components you think would break if only one channel was allocated ? We have been using it for almost a year now, so I think it's working well, but there may be some subtle audio impact that I'm not noticing. Only problem we are chasing is inconsistent comfort noise.

Thanks...

EDIT: oh yeah, with one channel, FIXED_POINT, we reduced opus encoder from 38k to about 29k.

judgeh avatar Oct 19 '20 15:10 judgeh

Silk was originally mono codec so it should work just fine by itself.

It's probably more integration issues inside Opus wrapper that might be assuming it to support stereo, similar to the bugs that you pointed out. But if you been running it for over a year it's probably fine.

@mark4o would fixes to the enc_API.c be accepted?

xnorpx avatar Oct 19 '20 15:10 xnorpx