diff mbox

[FFmpeg-devel] avcodec/h264_parser: Skip Redundant Slice Info Attached Behind Frames.

Message ID 20181025065916.18008-1-linjie.fu@intel.com
State New
Headers show

Commit Message

Fu, Linjie Oct. 25, 2018, 6:59 a.m. UTC
Skip redundant slice info attached behind frames (PPS for eaxmple) to
parse timestamp correctly and produce the segment format successfully.

When PPS info is attached behind frame data whin one PES packet,
h264_find_frame_end for PPS slice returns END_NOT_FOUND,and causes the
following IDR frame to returning END_NOT_FOUND too. And this leads to
the failure of parsing pts and operation of segment.

Skip redundant slice info to ensure the h264_parser to find the correct
frame_end of following frame, and make sure parse_packet could parse the
pts and dts.

Fix the pts issue for single segment.ts and the segment format
failure issue for http live stream in ticket #7207.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/h264_parser.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jun Zhao Oct. 25, 2018, 7:40 a.m. UTC | #1
On Thu, Oct 25, 2018 at 2:59 PM Linjie Fu <linjie.fu@intel.com> wrote:
>
> Skip redundant slice info attached behind frames (PPS for eaxmple) to
> parse timestamp correctly and produce the segment format successfully.
>
> When PPS info is attached behind frame data whin one PES packet,
> h264_find_frame_end for PPS slice returns END_NOT_FOUND,and causes the
> following IDR frame to returning END_NOT_FOUND too. And this leads to
> the failure of parsing pts and operation of segment.
>
> Skip redundant slice info to ensure the h264_parser to find the correct
> frame_end of following frame, and make sure parse_packet could parse the
> pts and dts.
>
> Fix the pts issue for single segment.ts and the segment format
> failure issue for http live stream in ticket #7207.
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/h264_parser.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> index 5f9a9c46ef..bcc97d6260 100644
> --- a/libavcodec/h264_parser.c
> +++ b/libavcodec/h264_parser.c
> @@ -577,6 +577,7 @@ static int h264_parse(AVCodecParserContext *s,
>      H264ParseContext *p = s->priv_data;
>      ParseContext *pc = &p->pc;
>      int next;
> +    int input_bufsize = buf_size;
>
>      if (!p->got_first) {
>          p->got_first = 1;
> @@ -644,7 +645,10 @@ static int h264_parse(AVCodecParserContext *s,
>
>      *poutbuf      = buf;
>      *poutbuf_size = buf_size;
> -    return next;
> +    if (next > 0 && next <input_bufsize)
> +        return input_bufsize;
> +    else
> +        return next;
>  }
>

You can use FFMIN(next, input_bufsize)  in this case.
Hendrik Leppkes Oct. 25, 2018, 9:40 a.m. UTC | #2
On Thu, Oct 25, 2018 at 8:59 AM Linjie Fu <linjie.fu@intel.com> wrote:
>
> Skip redundant slice info attached behind frames (PPS for eaxmple) to
> parse timestamp correctly and produce the segment format successfully.
>
> When PPS info is attached behind frame data whin one PES packet,
> h264_find_frame_end for PPS slice returns END_NOT_FOUND,and causes the
> following IDR frame to returning END_NOT_FOUND too. And this leads to
> the failure of parsing pts and operation of segment.
>
> Skip redundant slice info to ensure the h264_parser to find the correct
> frame_end of following frame, and make sure parse_packet could parse the
> pts and dts.
>
> Fix the pts issue for single segment.ts and the segment format
> failure issue for http live stream in ticket #7207.
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/h264_parser.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> index 5f9a9c46ef..bcc97d6260 100644
> --- a/libavcodec/h264_parser.c
> +++ b/libavcodec/h264_parser.c
> @@ -577,6 +577,7 @@ static int h264_parse(AVCodecParserContext *s,
>      H264ParseContext *p = s->priv_data;
>      ParseContext *pc = &p->pc;
>      int next;
> +    int input_bufsize = buf_size;
>
>      if (!p->got_first) {
>          p->got_first = 1;
> @@ -644,7 +645,10 @@ static int h264_parse(AVCodecParserContext *s,
>
>      *poutbuf      = buf;
>      *poutbuf_size = buf_size;
> -    return next;
> +    if (next > 0 && next <input_bufsize)
> +        return input_bufsize;
> +    else
> +        return next;
>  }
>
>  static int h264_split(AVCodecContext *avctx,

This doesn't seem right to me. The frames should be split in such a
way that the PPS ends up in front of the next frame, and not just
skipped at the end of the last one.
Instead of working around the problem of the next IDR not being read
properly, why not fix that problem?

- Hendrik
Hendrik Leppkes Oct. 25, 2018, 9:52 a.m. UTC | #3
On Thu, Oct 25, 2018 at 11:40 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Thu, Oct 25, 2018 at 8:59 AM Linjie Fu <linjie.fu@intel.com> wrote:
> >
> > Skip redundant slice info attached behind frames (PPS for eaxmple) to
> > parse timestamp correctly and produce the segment format successfully.
> >
> > When PPS info is attached behind frame data whin one PES packet,
> > h264_find_frame_end for PPS slice returns END_NOT_FOUND,and causes the
> > following IDR frame to returning END_NOT_FOUND too. And this leads to
> > the failure of parsing pts and operation of segment.
> >
> > Skip redundant slice info to ensure the h264_parser to find the correct
> > frame_end of following frame, and make sure parse_packet could parse the
> > pts and dts.
> >
> > Fix the pts issue for single segment.ts and the segment format
> > failure issue for http live stream in ticket #7207.
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> >  libavcodec/h264_parser.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> > index 5f9a9c46ef..bcc97d6260 100644
> > --- a/libavcodec/h264_parser.c
> > +++ b/libavcodec/h264_parser.c
> > @@ -577,6 +577,7 @@ static int h264_parse(AVCodecParserContext *s,
> >      H264ParseContext *p = s->priv_data;
> >      ParseContext *pc = &p->pc;
> >      int next;
> > +    int input_bufsize = buf_size;
> >
> >      if (!p->got_first) {
> >          p->got_first = 1;
> > @@ -644,7 +645,10 @@ static int h264_parse(AVCodecParserContext *s,
> >
> >      *poutbuf      = buf;
> >      *poutbuf_size = buf_size;
> > -    return next;
> > +    if (next > 0 && next <input_bufsize)
> > +        return input_bufsize;
> > +    else
> > +        return next;
> >  }
> >
> >  static int h264_split(AVCodecContext *avctx,
>
> This doesn't seem right to me. The frames should be split in such a
> way that the PPS ends up in front of the next frame, and not just
> skipped at the end of the last one.
> Instead of working around the problem of the next IDR not being read
> properly, why not fix that problem?
>

Since the original issue is about timestamps, this reminded me of this
patch that I've had sitting around for a while:
https://pastebin.com/ufE6nkWr

It was originally designed to help retrieve proper timestamps when
PPS/SPS are badly sligned to wrong PES  units, maybe it also helps
with your case.
However it was never included in the main codebase because it seemed
to also cause some other issues that I didnt have the time back then
to investigate.

- Hendrik
Fu, Linjie Oct. 25, 2018, 1:04 p.m. UTC | #4
> On Oct 25, 2018, at 17:53, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> 
>> On Thu, Oct 25, 2018 at 11:40 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> 
>>> On Thu, Oct 25, 2018 at 8:59 AM Linjie Fu <linjie.fu@intel.com> wrote:
>>> 
>>> Skip redundant slice info attached behind frames (PPS for eaxmple) to
>>> parse timestamp correctly and produce the segment format successfully.
>>> 
>>> When PPS info is attached behind frame data whin one PES packet,
>>> h264_find_frame_end for PPS slice returns END_NOT_FOUND,and causes the
>>> following IDR frame to returning END_NOT_FOUND too. And this leads to
>>> the failure of parsing pts and operation of segment.
>>> 
>>> Skip redundant slice info to ensure the h264_parser to find the correct
>>> frame_end of following frame, and make sure parse_packet could parse the
>>> pts and dts.
>>> 
>>> Fix the pts issue for single segment.ts and the segment format
>>> failure issue for http live stream in ticket #7207.
>>> 
>>> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
>>> ---
>>> libavcodec/h264_parser.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
>>> index 5f9a9c46ef..bcc97d6260 100644
>>> --- a/libavcodec/h264_parser.c
>>> +++ b/libavcodec/h264_parser.c
>>> @@ -577,6 +577,7 @@ static int h264_parse(AVCodecParserContext *s,
>>>     H264ParseContext *p = s->priv_data;
>>>     ParseContext *pc = &p->pc;
>>>     int next;
>>> +    int input_bufsize = buf_size;
>>> 
>>>     if (!p->got_first) {
>>>         p->got_first = 1;
>>> @@ -644,7 +645,10 @@ static int h264_parse(AVCodecParserContext *s,
>>> 
>>>     *poutbuf      = buf;
>>>     *poutbuf_size = buf_size;
>>> -    return next;
>>> +    if (next > 0 && next <input_bufsize)
>>> +        return input_bufsize;
>>> +    else
>>> +        return next;
>>> }
>>> 
>>> static int h264_split(AVCodecContext *avctx,
>> 
>> This doesn't seem right to me. The frames should be split in such a
>> way that the PPS ends up in front of the next frame, and not just
>> skipped at the end of the last one.
>> Instead of working around the problem of the next IDR not being read
>> properly, why not fix that problem?
>> 
> 
Thanks for your review. 

In this case, the skipped PPS is duplicated and useless. The next IDR frame has its own SPS and PPS and could be read properly after applying this patch, so the problem is fixed.

> Since the original issue is about timestamps, this reminded me of this
> patch that I've had sitting around for a while:
> https://pastebin.com/ufE6nkWr
> 
> It was originally designed to help retrieve proper timestamps when
> PPS/SPS are badly sligned to wrong PES  units, maybe it also helps
> with your case.
> However it was never included in the main codebase because it seemed
> to also cause some other issues that I didnt have the time back then
> to investigate.
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Actually, what I wonder is whether the PPS info attached at the end of a PES packet is useless in all situations. If the PPS is useful and but is only in wrong PES unit, it’s necessary to split the info.

Thanks again and will try your patch.
diff mbox

Patch

diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 5f9a9c46ef..bcc97d6260 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -577,6 +577,7 @@  static int h264_parse(AVCodecParserContext *s,
     H264ParseContext *p = s->priv_data;
     ParseContext *pc = &p->pc;
     int next;
+    int input_bufsize = buf_size;
 
     if (!p->got_first) {
         p->got_first = 1;
@@ -644,7 +645,10 @@  static int h264_parse(AVCodecParserContext *s,
 
     *poutbuf      = buf;
     *poutbuf_size = buf_size;
-    return next;
+    if (next > 0 && next <input_bufsize)
+        return input_bufsize;
+    else
+        return next;
 }
 
 static int h264_split(AVCodecContext *avctx,