[opus] Patches for adding 120 ms encoding

[Mark Harris]

Hi Felicia,

A few comments:

> -       /* CELT can only support up to 20 ms */
>         subframe_size = st->Fs/50;
> -       nb_subframes = frame_size > st->Fs/25 ? 3 : 2;
> +       nb_subframes = frame_size/subframe_size;

This will use six 20ms frames to make a 120ms packet, even for
SILK-only mode where frames can be up to 60ms.  For SILK, two 60ms
frames would be a more efficient way to encode a 120ms packet.  Also
FEC, if enabled, would be 3 times as effective.  Similarly, two 40ms
SILK frames would be more efficient than four 20ms SILK frames.


-    /* Can't support higher than wideband for >20 ms frames */
-    if (frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||
st->bandwidth > OPUS_BANDWIDTH_WIDEBAND))
+    /* Can't support higher than wideband for >20 ms frames,
CELT-only for >20 ms and SILK-only for >60 ms */
+    if ((frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||
st->bandwidth > OPUS_BANDWIDTH_WIDEBAND)) ||
+        (frame_size > 3*st->Fs/50 && st->mode == MODE_SILK_ONLY))

At this point in the function st->mode is not yet finalized; it has
only decided whether to use CELT-only mode.  If st->mode ==
MODE_CELT_ONLY then it has decided to use CELT-only mode.  Otherwise
it has decided to use SILK-only or Hybrid mode, and will set st->mode
to the correct mode (depending on bandwidth) a few lines after the end
of this if statement.  So unless those lines are moved before this if
statement, it doesn't make sense to compare st->mode == MODE_SILK_ONLY
here.

The "&& st->mode == MODE_SILK_ONLY" term is actually not needed at all
because you will want this if-condition to be true for any size larger
than 60ms regardless of mode.  Nevertheless you may still want to move
up the lines that finalize st->mode if you want to use it to determine
the size of each frame as mentioned above.  That would also allow this
if-condition to be further simplified.


> +static opus_int32 encode_subframes_and_repacketize(OpusEncoder *st,
> +                                const opus_val16 *pcm,
> +                                int nb_frames,
> +                                int frame_size,
> +                                unsigned char *data,
> +                                opus_int32 out_data_bytes,
> +                                int to_celt,
> +                                int lsb_depth,
> +                                int c1,
> +                                int c2,
> +                                int analysis_channels,
> +                                downmix_func downmix,
> +                                int float_api)

I understand that this was split out into a separate function
originally because you wanted to call it twice, but now that you have
merged the two calls is there still a need for it to be split out into
a separate function?  If it had a simple and concise interface then it
may make sense even with one call, but in this case the arguments that
it requires are numerous and peculiar to the specific implementation
in the calling function.


> +   bytes_per_frame = IMIN(1276,(out_data_bytes-3)/nb_frames);

The current code uses this formula because with up to 3 frames per
packet, in the worst case the combined packet will require
nb_frames*bytes_per_frame + 3 bytes (where bytes_per_frame is the code
0 packet length, as it is here).  However the worst case is worse with
more frames per packet.  A slight change to the formula would make it
sufficient for any valid number of frames per packet:

    bytes_per_frame = IMIN(1276, out_data_bytes/nb_frames - 1);


 - Mark


On Fri, Jun 10, 2016 at 7:35 PM, Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:
> Hi Felicia,
>
> I still need to look very carefully, which will take some time. That
> being said, some comments I can already make:
> 1) You need to also update the multi-stream API.
> 2) You might want to check that CBR works for >60 ms encoding
>
> Cheers,
>
>         Jean-Marc
>
>
> On 06/10/2016 10:19 AM, Felicia Lim wrote:
>> Hi, I wondered if are there any further thoughts on these patches?
>>
>> Thanks,
>> Felicia
>>
>> On Thu, Jun 2, 2016 at 2:13 PM Felicia Lim <flim at google.com
>> <mailto:flim at google.com>> wrote:
>>
>>     OK, I've amended the second patch and also added 80 and 100 ms.
>>
>>     Thanks,
>>     Felicia
>>
>>
>>     On Thu, Jun 2, 2016 at 7:20 AM Jean-Marc Valin <jmvalin at jmvalin.ca
>>     <mailto:jmvalin at jmvalin.ca>> wrote:
>>
>>         On 06/01/2016 02:06 PM, Felicia Lim wrote:
>>         > That was my intention with refactoring out the subframe
>>         encoding and
>>         > repacketizing bit. Or do you mean I should merge the explicit
>>         check for
>>         > 120 ms frame and the existing checks for 40/60 ms wideband?
>>
>>         What I mean is that this line in opus_encoder.c:
>>
>>         if (frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||
>>         st->bandwidth > OPUS_BANDWIDTH_WIDEBAND))
>>
>>         can probably be extended to also cover 80/100/120 ms. One
>>         difference is
>>         that it would also need to trigger for SILK-only > 60 ms.
>>
>>         Cheers,
>>
>>                 Jean-Marc
>>
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus