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. - /* 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.> +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.> + 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); - 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. > 2) You might want to check that CBR works for >60 ms encoding > > 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
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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20160613/a25dde3a/attachment.html>
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 >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20160627/96bb6708/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-Extend-support-for-80-100-and-120-ms.patch Type: application/octet-stream Size: 13655 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20160627/96bb6708/attachment-0001.obj>