Bug#772877: oggenc fails when using '--advanced-encode-option disable_coupling' switch and CBR encoding

[Martin Steghöfer]

reassign 772877 libvorbisenc2
retitle 772877 libvorbis: Segmentation fault when disabling channel coupling
thanks

Actually the oggenc code about setting the "disable_coupling" option 
seems to be fine, but the relevant libvorbis encoding code not so much. 
So I'm reassigning this to libvorbisenc2.

The code can be reproduced without oggenc in a simple example by calling 
"vorbis_encode_ctl" with action "OV_ECTL_COUPLING_SET" and argument 
being a pointer to an int variable with value 0.

I've found the following two problems with the encoding library.

First of all, the segmentation fault seems to occur due to poor return 
code checking:

/
| new_template = get_setup_template(.....);
| if(!hi->setup)return OV_EIMPL;
| hi->setup=new_template;
| hi->base_setting=new_base;
| vorbis_encode_setup_setting(vi,vi->channels,vi->rate);
\

If "get_setup_template" returns NULL, which it is allowed to in general, 
and which it does in this specific scenario, then "hi->setup" will be 
NULL - which "vorbis_encode_setup_setting" cannot cope with. The NULL 
check should either come *after* assigning the new value to "hi->setup" 
(that's the way it is done in "vorbis_encode_setup_vbr" and 
"vorbis_encode_setup_managed"), or the NULL check should refer to 
"new_template".

The second question is, why "get_setup_template" actually returns NULL. 
In this case, it should be able to find a matching setup template. The 
answer is in the following line at the beginning of "get_setup_template":

/
| if(q_or_bitrate)req/=ch;
\

This line is supposed to divide the total desired bitrate up into 
bitrates for each channel. However, in the specific case of disabling 
the coupling, the parameter "ch" is set to -1. The desired effect of 
this is to accept only setups that don't couple channels, which is 
achieved. However, as a side effect this also screws up the mentioned 
bitrate division. The bitrate is then divided by "-1" and we end up with 
a negative desired bitrate, which can never be fulfilled. Fixing this 
requires a signature change of "get_setup_template", otherwise the real 
number of channels cannot be known in this case.

Since the function "get_setup_template" doesn't appear in any header 
file at all, this signature change doesn't affect the ABI nor API and is 
therefore safe.

I will provide a patch, when I find the time to do a thorough testing of 
the fix.

Cheers,
Martin