[opus] Antw: Re: Patches for adding 120 ms encoding
Ulrich Windl
Ulrich.Windl at rz.uni-regensburg.de
Mon Jun 27 14:02:27 UTC 2016
Hi!
A note on style: Looking at this chunk of the patch
--
@@ -382,9 +382,15 @@ int main(int argc, char *argv[])
frame_size = sampling_rate/25;
else if (strcmp(argv[ args + 1 ], "60")==0)
frame_size = 3*sampling_rate/50;
+ else if (strcmp(argv[ args + 1 ], "80")==0)
+ frame_size = 4*sampling_rate/50;
+ else if (strcmp(argv[ args + 1 ], "100")==0)
+ frame_size = 5*sampling_rate/50;
+ else if (strcmp(argv[ args + 1 ], "120")==0)
+ frame_size = 6*sampling_rate/50;
--
I wonder whether it wouldn't be better style to convert argv[ args + 1 ] to a number once and then use a switch/case statements to test for individual numbers. Or is it expected to be non-numerical once?
Regards,
Ulrich
>>> Felicia Lim <flim at google.com> schrieb am 27.06.2016 um 15:05 in Nachricht
<CAJ0LFHW97AJ0J4EsaV2yO+EjJKnz8F47w1Te6_0y4tLA6xAmsw at mail.gmail.com>:
> Attached is the amended second patch. It now extends the multistream API as
> well to 80/100/120 ms and incorporates changes based on Mark's comments.
>
> Thanks,
> Felicia
>
> On Mon, Jun 13, 2016 at 4:21 PM Felicia Lim <flim at google.com> wrote:
>
>> Hi Mark, Jean-Marc,
>>
>> Thanks for your comments.
>>
>> On Sun, Jun 12, 2016 at 6:34 AM Mark Harris <mark.hsj at gmail.com> wrote:
>>
>>> 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.
>>>
>>
>> That makes sense, I've changed it so that SILK 120/80 ms encode 2x 60 and
>> 2x 40 ms respectively.
>>
>>
>>>
>>>
>>> - /* 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.
>>>
>>>
>> Thanks for pointing this out. I've moved the bandwidth-dependent
>> SILK/Hybrid mode decision up and simplified the if-condition as:
>>
>> if ((frame_size > st->Fs/50 && (st->mode != MODE_SILK_ONLY)) || frame_size
>> > 3*st->Fs/50)
>>
>>
>>>
>>> > +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.
>>>
>>>
>> I opted to leave this anyway because the logic here seemed significant
>> enough to stand by itself and it makes the purpose of this code more
>> explicit in opus_encode_native. I agree though that there are a lot of
>> input arguments (it could be reduced by 1 by moving the nr_subframes
>> calculation in here as well). I can also undo this split if this is
>> preferred?
>>
>>
>>>
>>> > + 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);
>>>
>>>
>> This has now been fixed.
>>
>>
>>>
>>> - 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.
>>
>>
>> Agreed, I had originally planned to wait until the single-stream patch
>> looked good to split the patches/review into smaller chunks, but I'm also
>> happy to start on the multi-stream API and share that together with the
>> single-stream patches?
>>
>> > 2) You might want to check that CBR works for >60 ms encoding
>>>
>>
>> I have checked that the packet lengths returned by the encoder are
>> constant with the expected values, based on the requested bitrate.
>>
>> Thanks,
>> Felicia
>>
>>
>>> >
>>> > 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
>>>
>>
More information about the opus
mailing list