<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>