| Message ID | 20210621160535.71659-1-gsun@roblox.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avformat/oggparsevorbis: Update end_trimming for the last packet | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | fail | Make fate failed |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | warning | Make fate failed |
On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn@gmail.com> wrote: > > From: Guangyu Sun <sunguangyucn@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@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.
6 Jul 2021, 08:02 by sunguangyucn@gmail.com: > On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn@gmail.com> wrote: > >> >> From: Guangyu Sun <sunguangyucn@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@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.
On Tue, Jul 6, 2021 at 1:15 AM Lynne <dev@lynne.ee> wrote: > > 6 Jul 2021, 08:02 by sunguangyucn@gmail.com: > > > On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn@gmail.com> wrote: > > > >> > >> From: Guangyu Sun <sunguangyucn@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@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 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
6 Jul 2021, 18:21 by sunguangyucn@gmail.com: > On Tue, Jul 6, 2021 at 1:15 AM Lynne <dev@lynne.ee> wrote: > >> >> 6 Jul 2021, 08:02 by sunguangyucn@gmail.com: >> >> > On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn@gmail.com> wrote: >> > >> >> >> >> From: Guangyu Sun <sunguangyucn@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@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? 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.
On Wed, Jul 7, 2021 at 2:41 AM Lynne <dev@lynne.ee> wrote: > > 6 Jul 2021, 18:21 by sunguangyucn@gmail.com: > > > On Tue, Jul 6, 2021 at 1:15 AM Lynne <dev@lynne.ee> wrote: > > > >> > >> 6 Jul 2021, 08:02 by sunguangyucn@gmail.com: > >> > >> > On Mon, Jun 21, 2021 at 9:06 AM Guangyu Sun <sunguangyucn@gmail.com> wrote: > >> > > >> >> > >> >> From: Guangyu Sun <sunguangyucn@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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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; }