[opus] Antw: Re: Re: Patches for adding 120 ms encoding

Ulrich Windl Ulrich.Windl at rz.uni-regensburg.de
Tue Jun 28 13:45:54 UTC 2016


>>> Felicia Lim <flim at google.com> schrieb am 28.06.2016 um 12:28 in Nachricht
<CAJ0LFHW_9sjqT+BArM_ON7zgQ3-1Sk93BG57H3GcD62vQXZw8A at mail.gmail.com>:
> Hi Ulrich, thanks for the suggestion. My concern is that one of the valid
> inputs is "2.5", which would require conversion to an int, e.g. x10, but
> doing something like this would start to affect the code readability.

OK, so you want to allow floating point input while avoiding to use float in the code, right?
However I wonder how you match all possible floating point values using strcmp() ;-)

Or are you worried about the remaining ".5" after "2"? If so, that's somewhat easy:

---some code I wrote---
                        f = strtof(*argp, (char **) &next);
                        if ( *argp == next || next[0] != '/' )
                        {
                                bad_option_flag(option, flag, *argp - 1);
                                result = -1;
                        }
---end of code---

So if nothing was consumed or (in my case) the character after the number is not '/', it's an error.

Regards,
Ulrich

> 
> On Mon, Jun 27, 2016 at 3:02 PM Ulrich Windl <
> Ulrich.Windl at rz.uni-regensburg.de> wrote:
> 
>> 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