<div dir="ltr">Hi Ulrich, t<span style="line-height:1.5">hanks 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.</span><div><div><br></div><div><div class="gmail_quote"><div dir="ltr">On Mon, Jun 27, 2016 at 3:02 PM Ulrich Windl <<a href="mailto:Ulrich.Windl@rz.uni-regensburg.de">Ulrich.Windl@rz.uni-regensburg.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi!<br>
<br>
A note on style: Looking at this chunk of the patch<br>
--<br>
@@ -382,9 +382,15 @@ int main(int argc, char *argv[])<br>
frame_size = sampling_rate/25;<br>
else if (strcmp(argv[ args + 1 ], "60")==0)<br>
frame_size = 3*sampling_rate/50;<br>
+ else if (strcmp(argv[ args + 1 ], "80")==0)<br>
+ frame_size = 4*sampling_rate/50;<br>
+ else if (strcmp(argv[ args + 1 ], "100")==0)<br>
+ frame_size = 5*sampling_rate/50;<br>
+ else if (strcmp(argv[ args + 1 ], "120")==0)<br>
+ frame_size = 6*sampling_rate/50;<br>
--<br>
<br>
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?<br>
<br>
Regards,<br>
Ulrich<br>
<br>
>>> Felicia Lim <<a href="mailto:flim@google.com" target="_blank">flim@google.com</a>> schrieb am 27.06.2016 um 15:05 in Nachricht<br>
<<a href="mailto:CAJ0LFHW97AJ0J4EsaV2yO%2BEjJKnz8F47w1Te6_0y4tLA6xAmsw@mail.gmail.com" target="_blank">CAJ0LFHW97AJ0J4EsaV2yO+EjJKnz8F47w1Te6_0y4tLA6xAmsw@mail.gmail.com</a>>:<br>
> Attached is the amended second patch. It now extends the multistream API as<br>
> well to 80/100/120 ms and incorporates changes based on Mark's comments.<br>
><br>
> Thanks,<br>
> Felicia<br>
><br>
> On Mon, Jun 13, 2016 at 4:21 PM Felicia Lim <<a href="mailto:flim@google.com" target="_blank">flim@google.com</a>> wrote:<br>
><br>
>> Hi Mark, Jean-Marc,<br>
>><br>
>> Thanks for your comments.<br>
>><br>
>> On Sun, Jun 12, 2016 at 6:34 AM Mark Harris <<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a>> wrote:<br>
>><br>
>>> Hi Felicia,<br>
>>><br>
>>> A few comments:<br>
>>><br>
>>> > - /* CELT can only support up to 20 ms */<br>
>>> > subframe_size = st->Fs/50;<br>
>>> > - nb_subframes = frame_size > st->Fs/25 ? 3 : 2;<br>
>>> > + nb_subframes = frame_size/subframe_size;<br>
>>><br>
>>> This will use six 20ms frames to make a 120ms packet, even for<br>
>>> SILK-only mode where frames can be up to 60ms. For SILK, two 60ms<br>
>>> frames would be a more efficient way to encode a 120ms packet. Also<br>
>>> FEC, if enabled, would be 3 times as effective. Similarly, two 40ms<br>
>>> SILK frames would be more efficient than four 20ms SILK frames.<br>
>>><br>
>><br>
>> That makes sense, I've changed it so that SILK 120/80 ms encode 2x 60 and<br>
>> 2x 40 ms respectively.<br>
>><br>
>><br>
>>><br>
>>><br>
>>> - /* Can't support higher than wideband for >20 ms frames */<br>
>>> - if (frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||<br>
>>> st->bandwidth > OPUS_BANDWIDTH_WIDEBAND))<br>
>>> + /* Can't support higher than wideband for >20 ms frames,<br>
>>> CELT-only for >20 ms and SILK-only for >60 ms */<br>
>>> + if ((frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||<br>
>>> st->bandwidth > OPUS_BANDWIDTH_WIDEBAND)) ||<br>
>>> + (frame_size > 3*st->Fs/50 && st->mode == MODE_SILK_ONLY))<br>
>>><br>
>>> At this point in the function st->mode is not yet finalized; it has<br>
>>> only decided whether to use CELT-only mode. If st->mode ==<br>
>>> MODE_CELT_ONLY then it has decided to use CELT-only mode. Otherwise<br>
>>> it has decided to use SILK-only or Hybrid mode, and will set st->mode<br>
>>> to the correct mode (depending on bandwidth) a few lines after the end<br>
>>> of this if statement. So unless those lines are moved before this if<br>
>>> statement, it doesn't make sense to compare st->mode == MODE_SILK_ONLY<br>
>>> here.<br>
>>><br>
>>> The "&& st->mode == MODE_SILK_ONLY" term is actually not needed at all<br>
>>> because you will want this if-condition to be true for any size larger<br>
>>> than 60ms regardless of mode. Nevertheless you may still want to move<br>
>>> up the lines that finalize st->mode if you want to use it to determine<br>
>>> the size of each frame as mentioned above. That would also allow this<br>
>>> if-condition to be further simplified.<br>
>>><br>
>>><br>
>> Thanks for pointing this out. I've moved the bandwidth-dependent<br>
>> SILK/Hybrid mode decision up and simplified the if-condition as:<br>
>><br>
>> if ((frame_size > st->Fs/50 && (st->mode != MODE_SILK_ONLY)) || frame_size<br>
>> > 3*st->Fs/50)<br>
>><br>
>><br>
>>><br>
>>> > +static opus_int32 encode_subframes_and_repacketize(OpusEncoder *st,<br>
>>> > + const opus_val16 *pcm,<br>
>>> > + int nb_frames,<br>
>>> > + int frame_size,<br>
>>> > + unsigned char *data,<br>
>>> > + opus_int32 out_data_bytes,<br>
>>> > + int to_celt,<br>
>>> > + int lsb_depth,<br>
>>> > + int c1,<br>
>>> > + int c2,<br>
>>> > + int analysis_channels,<br>
>>> > + downmix_func downmix,<br>
>>> > + int float_api)<br>
>>><br>
>>> I understand that this was split out into a separate function<br>
>>> originally because you wanted to call it twice, but now that you have<br>
>>> merged the two calls is there still a need for it to be split out into<br>
>>> a separate function? If it had a simple and concise interface then it<br>
>>> may make sense even with one call, but in this case the arguments that<br>
>>> it requires are numerous and peculiar to the specific implementation<br>
>>> in the calling function.<br>
>>><br>
>>><br>
>> I opted to leave this anyway because the logic here seemed significant<br>
>> enough to stand by itself and it makes the purpose of this code more<br>
>> explicit in opus_encode_native. I agree though that there are a lot of<br>
>> input arguments (it could be reduced by 1 by moving the nr_subframes<br>
>> calculation in here as well). I can also undo this split if this is<br>
>> preferred?<br>
>><br>
>><br>
>>><br>
>>> > + bytes_per_frame = IMIN(1276,(out_data_bytes-3)/nb_frames);<br>
>>><br>
>>> The current code uses this formula because with up to 3 frames per<br>
>>> packet, in the worst case the combined packet will require<br>
>>> nb_frames*bytes_per_frame + 3 bytes (where bytes_per_frame is the code<br>
>>> 0 packet length, as it is here). However the worst case is worse with<br>
>>> more frames per packet. A slight change to the formula would make it<br>
>>> sufficient for any valid number of frames per packet:<br>
>>><br>
>>> bytes_per_frame = IMIN(1276, out_data_bytes/nb_frames - 1);<br>
>>><br>
>>><br>
>> This has now been fixed.<br>
>><br>
>><br>
>>><br>
>>> - Mark<br>
>>><br>
>>><br>
>>> On Fri, Jun 10, 2016 at 7:35 PM, Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>><br>
>>> wrote:<br>
>>> > Hi Felicia,<br>
>>> ><br>
>>> > I still need to look very carefully, which will take some time. That<br>
>>> > being said, some comments I can already make:<br>
>>> > 1) You need to also update the multi-stream API.<br>
>><br>
>><br>
>> Agreed, I had originally planned to wait until the single-stream patch<br>
>> looked good to split the patches/review into smaller chunks, but I'm also<br>
>> happy to start on the multi-stream API and share that together with the<br>
>> single-stream patches?<br>
>><br>
>> > 2) You might want to check that CBR works for >60 ms encoding<br>
>>><br>
>><br>
>> I have checked that the packet lengths returned by the encoder are<br>
>> constant with the expected values, based on the requested bitrate.<br>
>><br>
>> Thanks,<br>
>> Felicia<br>
>><br>
>><br>
>>> ><br>
>>> > Cheers,<br>
>>> ><br>
>>> > Jean-Marc<br>
>>> ><br>
>>> ><br>
>>> > On 06/10/2016 10:19 AM, Felicia Lim wrote:<br>
>>> >> Hi, I wondered if are there any further thoughts on these patches?<br>
>>> >><br>
>>> >> Thanks,<br>
>>> >> Felicia<br>
>>> >><br>
>>> >> On Thu, Jun 2, 2016 at 2:13 PM Felicia Lim <<a href="mailto:flim@google.com" target="_blank">flim@google.com</a><br>
>>> >> <mailto:<a href="mailto:flim@google.com" target="_blank">flim@google.com</a>>> wrote:<br>
>>> >><br>
>>> >> OK, I've amended the second patch and also added 80 and 100 ms.<br>
>>> >><br>
>>> >> Thanks,<br>
>>> >> Felicia<br>
>>> >><br>
>>> >><br>
>>> >> On Thu, Jun 2, 2016 at 7:20 AM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a><br>
>>> >> <mailto:<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>>> wrote:<br>
>>> >><br>
>>> >> On 06/01/2016 02:06 PM, Felicia Lim wrote:<br>
>>> >> > That was my intention with refactoring out the subframe<br>
>>> >> encoding and<br>
>>> >> > repacketizing bit. Or do you mean I should merge the explicit<br>
>>> >> check for<br>
>>> >> > 120 ms frame and the existing checks for 40/60 ms wideband?<br>
>>> >><br>
>>> >> What I mean is that this line in opus_encoder.c:<br>
>>> >><br>
>>> >> if (frame_size > st->Fs/50 && (st->mode == MODE_CELT_ONLY ||<br>
>>> >> st->bandwidth > OPUS_BANDWIDTH_WIDEBAND))<br>
>>> >><br>
>>> >> can probably be extended to also cover 80/100/120 ms. One<br>
>>> >> difference is<br>
>>> >> that it would also need to trigger for SILK-only > 60 ms.<br>
>>> >><br>
>>> >> Cheers,<br>
>>> >><br>
>>> >> Jean-Marc<br>
>>> >><br>
>>> > _______________________________________________<br>
>>> > opus mailing list<br>
>>> > <a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a><br>
>>> > <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
>>><br>
>><br>
<br>
<br>
<br>
</blockquote></div></div></div></div>