<div dir="ltr">Hi Mark, Jean-Marc,<div><br></div><div>Thanks for your comments. </div><div><br><div class="gmail_quote"><div dir="ltr">On Sun, Jun 12, 2016 at 6:34 AM Mark Harris <<a href="mailto:mark.hsj@gmail.com">mark.hsj@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div>That makes sense, I've changed it so that SILK 120/80 ms encode 2x 60 and 2x 40 ms respectively.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Thanks for pointing this out. I've moved the bandwidth-dependent SILK/Hybrid mode decision up and simplified the if-condition as:</div><div><br></div><div>if ((frame_size > st->Fs/50 && (st->mode != MODE_SILK_ONLY)) || frame_size > 3*st->Fs/50)<br></div><div><span style="line-height:1.5"> </span><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div> </div><div>This has now been fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>> 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.<span style="line-height:1.5"> </span></blockquote><div><br></div><div>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?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> 2) You might want to check that CBR works for >60 ms encoding<br></blockquote><div><br></div><div>I have checked that the packet lengths returned by the encoder are constant with the expected values, based on the requested bitrate.</div><div><br></div><div>Thanks,</div><div>Felicia</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><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>
</blockquote></div></div></div>