diff mbox series

[FFmpeg-devel] avcodec/h264_mp4toannexb: fix missing SPS/PPS to IDR frames

Message ID tencent_EE0E40DE9A569FE5AB454E6E700A2DA79A08@qq.com
State New
Headers show
Series [FFmpeg-devel] avcodec/h264_mp4toannexb: fix missing SPS/PPS to IDR frames | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Zhao Zhili Dec. 28, 2022, 5:20 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

If there is a single group of SPS/PPS before an IDR frame, but no
SPS/PPS after that, we will miss the chance to reset
idr_sps_seen/idr_pps_seen. The result is missing SPS/PPS for all
IDR frames except the first one.

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
 libavcodec/h264_mp4toannexb_bsf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Zhao Zhili Jan. 9, 2023, 4:46 p.m. UTC | #1
On Thu, 2022-12-29 at 01:20 +0800, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> If there is a single group of SPS/PPS before an IDR frame, but no
> SPS/PPS after that, we will miss the chance to reset
> idr_sps_seen/idr_pps_seen. The result is missing SPS/PPS for all
> IDR frames except the first one.
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavcodec/h264_mp4toannexb_bsf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h264_mp4toannexb_bsf.c
> b/libavcodec/h264_mp4toannexb_bsf.c
> index d11be455c2..bbdeca9c44 100644
> --- a/libavcodec/h264_mp4toannexb_bsf.c
> +++ b/libavcodec/h264_mp4toannexb_bsf.c
> @@ -259,7 +259,7 @@ static int h264_mp4toannexb_filter(AVBSFContext
> *ctx, AVPacket *opkt)
>  
>              count_or_copy(&out, &out_size, buf, nal_size,
>                            unit_type == H264_NAL_SPS || unit_type ==
> H264_NAL_PPS, j);
> -            if (!new_idr && unit_type == H264_NAL_SLICE) {
> +            if (unit_type == H264_NAL_SLICE) {
>                  new_idr  = 1;
>                  sps_seen = 0;
>                  pps_seen = 0;

Ping for review.
Andreas Rheinhardt Jan. 10, 2023, 6:41 a.m. UTC | #2
Zhao Zhili:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> If there is a single group of SPS/PPS before an IDR frame, but no
> SPS/PPS after that, we will miss the chance to reset
> idr_sps_seen/idr_pps_seen. The result is missing SPS/PPS for all
> IDR frames except the first one.
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavcodec/h264_mp4toannexb_bsf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
> index d11be455c2..bbdeca9c44 100644
> --- a/libavcodec/h264_mp4toannexb_bsf.c
> +++ b/libavcodec/h264_mp4toannexb_bsf.c
> @@ -259,7 +259,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>  
>              count_or_copy(&out, &out_size, buf, nal_size,
>                            unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
> -            if (!new_idr && unit_type == H264_NAL_SLICE) {
> +            if (unit_type == H264_NAL_SLICE) {
>                  new_idr  = 1;
>                  sps_seen = 0;
>                  pps_seen = 0;

If I understand you correctly, you want that to include the parameter
sets from the extradata to every IDR frame without extradata if said IDR
frame follows a non-IDR frame after in-band parameter sets. There is
just one problem with this: Said in-band parameter sets might overwrite
parameter sets from extradata and inserting the outdated parameter sets
from extradata lateron might make the stream undecodable.

- Andreas
Zhao Zhili Jan. 10, 2023, 3:31 p.m. UTC | #3
On Tue, 2023-01-10 at 07:41 +0100, Andreas Rheinhardt wrote:
> Zhao Zhili:
> > From: Zhao Zhili <zhilizhao@tencent.com>
> > 
> > If there is a single group of SPS/PPS before an IDR frame, but no
> > SPS/PPS after that, we will miss the chance to reset
> > idr_sps_seen/idr_pps_seen. The result is missing SPS/PPS for all
> > IDR frames except the first one.
> > 
> > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > ---
> >  libavcodec/h264_mp4toannexb_bsf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/h264_mp4toannexb_bsf.c
> > b/libavcodec/h264_mp4toannexb_bsf.c
> > index d11be455c2..bbdeca9c44 100644
> > --- a/libavcodec/h264_mp4toannexb_bsf.c
> > +++ b/libavcodec/h264_mp4toannexb_bsf.c
> > @@ -259,7 +259,7 @@ static int h264_mp4toannexb_filter(AVBSFContext
> > *ctx, AVPacket *opkt)
> >  
> >              count_or_copy(&out, &out_size, buf, nal_size,
> >                            unit_type == H264_NAL_SPS || unit_type
> > == H264_NAL_PPS, j);
> > -            if (!new_idr && unit_type == H264_NAL_SLICE) {
> > +            if (unit_type == H264_NAL_SLICE) {
> >                  new_idr  = 1;
> >                  sps_seen = 0;
> >                  pps_seen = 0;
> 
> If I understand you correctly, you want that to include the parameter
> sets from the extradata to every IDR frame without extradata if said
> IDR
> frame follows a non-IDR frame after in-band parameter sets. There is
> just one problem with this: Said in-band parameter sets might
> overwrite
> parameter sets from extradata and inserting the outdated parameter
> sets
> from extradata lateron might make the stream undecodable.

It's a good question. Current code works when an IDR has in-band
parameter set, which is different than the out-of-band parameter set,
and the decoder doesn't miss the in-band parameter set. But there is a
high chance the decoder will miss the in-band parameter set for annexb
bitstream, e.g., playback at the middle of a live mpegts streams. It's
a serious issue of missing periodic in-band parameter set for annexb
for a lot of usecases. Handle new parameter set isn't as common as the
need of periodic in-band parameter set (I guess).

It's not a new issue that this bitstream failed to update the parameter
set, for example, it doesn't handle AV_PKT_DATA_NEW_EXTRADATA. I can
add support for
1. handling AV_PKT_DATA_NEW_EXTRADATA
2. handling in-band sps/pps

But it won't be soon.

> 
> - Andreas
> 
> _______________________________________________
> 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".
Zhao Zhili Jan. 16, 2023, 3:23 a.m. UTC | #4
> On Jan 10, 2023, at 23:31, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> On Tue, 2023-01-10 at 07:41 +0100, Andreas Rheinhardt wrote:
>> Zhao Zhili:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> If there is a single group of SPS/PPS before an IDR frame, but no
>>> SPS/PPS after that, we will miss the chance to reset
>>> idr_sps_seen/idr_pps_seen. The result is missing SPS/PPS for all
>>> IDR frames except the first one.
>>> 
>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>>> ---
>>> libavcodec/h264_mp4toannexb_bsf.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/libavcodec/h264_mp4toannexb_bsf.c
>>> b/libavcodec/h264_mp4toannexb_bsf.c
>>> index d11be455c2..bbdeca9c44 100644
>>> --- a/libavcodec/h264_mp4toannexb_bsf.c
>>> +++ b/libavcodec/h264_mp4toannexb_bsf.c
>>> @@ -259,7 +259,7 @@ static int h264_mp4toannexb_filter(AVBSFContext
>>> *ctx, AVPacket *opkt)
>>> 
>>>             count_or_copy(&out, &out_size, buf, nal_size,
>>>                           unit_type == H264_NAL_SPS || unit_type
>>> == H264_NAL_PPS, j);
>>> -            if (!new_idr && unit_type == H264_NAL_SLICE) {
>>> +            if (unit_type == H264_NAL_SLICE) {
>>>                 new_idr  = 1;
>>>                 sps_seen = 0;
>>>                 pps_seen = 0;
>> 
>> If I understand you correctly, you want that to include the parameter
>> sets from the extradata to every IDR frame without extradata if said
>> IDR
>> frame follows a non-IDR frame after in-band parameter sets. There is
>> just one problem with this: Said in-band parameter sets might
>> overwrite
>> parameter sets from extradata and inserting the outdated parameter
>> sets
>> from extradata lateron might make the stream undecodable.
> 
> It's a good question. Current code works when an IDR has in-band
> parameter set, which is different than the out-of-band parameter set,
> and the decoder doesn't miss the in-band parameter set. But there is a
> high chance the decoder will miss the in-band parameter set for annexb
> bitstream, e.g., playback at the middle of a live mpegts streams. It's
> a serious issue of missing periodic in-band parameter set for annexb
> for a lot of usecases. Handle new parameter set isn't as common as the
> need of periodic in-band parameter set (I guess).
> 
> It's not a new issue that this bitstream failed to update the parameter
> set, for example, it doesn't handle AV_PKT_DATA_NEW_EXTRADATA. I can
> add support for
> 1. handling AV_PKT_DATA_NEW_EXTRADATA
> 2. handling in-band sps/pps
> 
> But it won't be soon.

Ping.

> 
>> 
>> - Andreas
>> 
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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 mbox series

Patch

diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
index d11be455c2..bbdeca9c44 100644
--- a/libavcodec/h264_mp4toannexb_bsf.c
+++ b/libavcodec/h264_mp4toannexb_bsf.c
@@ -259,7 +259,7 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
 
             count_or_copy(&out, &out_size, buf, nal_size,
                           unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
-            if (!new_idr && unit_type == H264_NAL_SLICE) {
+            if (unit_type == H264_NAL_SLICE) {
                 new_idr  = 1;
                 sps_seen = 0;
                 pps_seen = 0;