[FFmpeg-devel] [PATCH] avformat/oggparsevorbis: Update end_trimming for the last packet

[Guangyu Sun]

On Wed, Jul 7, 2021 at 2:41 AM Lynne <dev at lynne.ee> wrote:
>
> 6 Jul 2021, 18:21 by sunguangyucn at gmail.com:
>
> > On Tue, Jul 6, 2021 at 1:15 AM Lynne <dev at lynne.ee> wrote:
> >
> >>
> >> 6 Jul 2021, 08:02 by sunguangyucn at gmail.com:
> >>
> >> > On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn at gmail.com> wrote:
> >> >
> >> >>
> >> >> From: Guangyu Sun <sunguangyucn at gmail.com>
> >> >>
> >> >> Without end_trimming, the last packet will contain unexpected samples used
> >> >> for padding.
> >> >>
> >> >> This commit partially fixes #6367 when the audio length is long enough.
> >> >>
> >> >> dd if=/dev/zero of=./silence.raw count=20 bs=500
> >> >> oggenc --raw silence.raw --output=silence.ogg
> >> >> oggdec --raw --output silence.oggdec.raw silence.ogg
> >> >> ffmpeg -codec:a libvorbis -i silence.ogg -f s16le -codec:a pcm_s16le silence.libvorbis.ffmpeg.raw
> >> >> ffmpeg -i silence.ogg -f s16le -codec:a pcm_s16le silence.native.ffmpeg.raw
> >> >> ls -l *.raw
> >> >>
> >> >> The original test case in #6367 is still not fixed due to a remaining issue.
> >> >>
> >> >> The remaining issue is that ogg_stream->private is not kept during
> >> >> ogg_save()/ogg_restore(). Field final_duration in the private data is
> >> >> important to calculate end_trimming.
> >> >>
> >> >> Some common operations such as avformat_open_input() and
> >> >> avformat_find_stream_info() before reading packet will trigger ogg_save()
> >> >> and ogg_restore().
> >> >>
> >> >> Luckily, final_duration will not get updated until the last ogg page. The
> >> >> save/restore mentioned above will not change final_duration most of the
> >> >> time. But if the audio length is short, those reads may be performed on
> >> >> the last ogg page, causing trouble keeping the correct value of
> >> >> final_duration. We probably need a more complicated patch to address this
> >> >> issue.
> >> >>
> >> >> Signed-off-by: Guangyu Sun <sunguangyucn at gmail.com>
> >> >> ---
> >> >>  libavformat/oggparsevorbis.c | 6 +++++-
> >> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> >> >> index 0e8c25c030..c48658ceda 100644
> >> >> --- a/libavformat/oggparsevorbis.c
> >> >> +++ b/libavformat/oggparsevorbis.c
> >> >> @@ -492,8 +492,12 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> >> >>  priv->final_pts      = os->lastpts;
> >> >>  priv->final_duration = 0;
> >> >>  }
> >> >> -        if (os->segp == os->nsegs)
> >> >> +        if (os->segp == os->nsegs) {
> >> >> +            int64_t skip = priv->final_pts + priv->final_duration + os->pduration - os->granule;
> >> >> +            if (skip > 0)
> >> >> +                os->end_trimming = skip;
> >> >>  os->pduration = os->granule - priv->final_pts - priv->final_duration;
> >> >> +        }
> >> >>  priv->final_duration += os->pduration;
> >> >>  }
> >> >>
> >> >> --
> >> >> 2.30.1
> >> >>
> >> >
> >> > After fixing AV_PKT_DATA_SKIP_SAMPLES for reading vorbis packets from
> >> > ogg, the actual decoded samples become fewer. As a result, three fate
> >> > tests are failing:
> >> >
> >> > fate-vorbis-encode:
> >> > This exposes another bug in the vorbis encoder that initial_padding is
> >> > not set. I will fix that in a separate patch.
> >> >
> >> > fate-webm-dash-chapters:
> >> > The original vorbis_chapter_extension_demo.ogg is transmuxed to
> >> > dash-webm. The ref file webm-dash-chapters needs to be updated.
> >> >
> >> > fate-vorbis-20:
> >> > The samples in 6.ogg are not frame aligned. 6.pcm file was generated
> >> > by ffmpeg before the fix. After the fix, the decoded pcm file does not
> >> > match anymore. The ref file 6.pcm needs to be updated.
> >> >
> >> > My question is that to fix fate-vorbis-20, a new file 6_v2.pcm needs
> >> > to be uploaded to the fate suite server. How should I do that? The doc
> >> > says it should be sent to samples-request but I am not sure if it is
> >> > an email or something else.
> >> >
> >>
> >> Just put a link to it here and someone will upload it. Then a day will have
> >> to pass to let all fate clients pull it before pushing the patch.
> >> Patch looks good to me as-is, since it's just a copy of what Opus does.
> >>
> >
> > Great! Here is the link:
> > https://tinyurl.com/hkmwdk78
> >
> > I would like it to be uploaded to:
> > fate-suite/vorbis/6_v2.pcm
> >
> > Thanks in advance!
> >
> > Best,
> > Guangyu
> >
>
> Could you send the rest of the fixes as well, for the encoder and fate test?
Sure. I was holding it because the fate test patch assumes the new
file 6_v2.pcm is already uploaded. I will send all 3 of them together
shortly.

Thanks,
Guangyu

> There's no point in applying this if it breaks fate until you submit new patches to fix it.
> Also, the difference in the file is 9948 bytes, which divided by 2 bytes
> per sample times 6 channels make 829 samples. This sounds correct,
> so the new file makes sense.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".