h264_parser: don't stop on SPS_EXT in split

Message ID 1414681524-26451-1-git-send-email-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Oct. 30, 2014, 3:05 p.m.
SPS_EXT can come before the PPS and results in incomplete extradata.
---
 libavcodec/h264_parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Hendrik Leppkes Oct. 30, 2014, 3:20 p.m. | #1
On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
wrote:

> SPS_EXT can come before the PPS and results in incomplete extradata.
>

If you want this to be really complete, may also want to add SEI (6) and
subset SPS (15), I had some sort of crazy file where this was required to
get the SPS/PPS into extradata, but I lost it.

- Hendrik
John Stebbins Oct. 30, 2014, 3:45 p.m. | #2
On 10/30/2014 08:20 AM, Hendrik Leppkes wrote:
> On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
> wrote:
>
>> SPS_EXT can come before the PPS and results in incomplete extradata.
>>
> If you want this to be really complete, may also want to add SEI (6) and
> subset SPS (15), I had some sort of crazy file where this was required to
> get the SPS/PPS into extradata, but I lost it.
>
>

Does it make sense to drop all these NAL checks and just add another flag has_pps that gets set when PPS is seen?  Or is
this already known to cause problems?  It would be a performance hit if there is no PPS in the packet, but split isn't
called often.
Hendrik Leppkes Oct. 30, 2014, 4:19 p.m. | #3
On Thu, Oct 30, 2014 at 4:45 PM, John Stebbins <stebbins@jetheaddev.com>
wrote:

> On 10/30/2014 08:20 AM, Hendrik Leppkes wrote:
> > On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
> > wrote:
> >
> >> SPS_EXT can come before the PPS and results in incomplete extradata.
> >>
> > If you want this to be really complete, may also want to add SEI (6) and
> > subset SPS (15), I had some sort of crazy file where this was required to
> > get the SPS/PPS into extradata, but I lost it.
> >
> >
>
> Does it make sense to drop all these NAL checks and just add another flag
> has_pps that gets set when PPS is seen?  Or is
> this already known to cause problems?  It would be a performance hit if
> there is no PPS in the packet, but split isn't
> called often.
>
>
The whole idea is to stop working when it finds something that absolutely
means there isn't going to be any metadata anymore, like, a slice.
So the check does serve some purpose to avoid parsing through the entire
frame, just needs to be a bit more relaxed and allow a few more metadata
NALs.

Additionally to stoping parsing, it also ensures a slice isn't copied into
extradata in some crazy bitstream condition.

- Hendrik
John Stebbins Oct. 31, 2014, 2:48 p.m. | #4
On 10/30/2014 08:20 AM, Hendrik Leppkes wrote:
> On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
> wrote:
>
>> SPS_EXT can come before the PPS and results in incomplete extradata.
>>
> If you want this to be really complete, may also want to add SEI (6) and
> subset SPS (15), I had some sort of crazy file where this was required to
> get the SPS/PPS into extradata, but I lost it.
>
>

Subset SPS is in a "reserved" range according to my copy of the h.264 spec (2005).  Has there been an update that adds
this NAL?  If so, should we update h264.h with the new NAL code(s)?
Vittorio Giovara Oct. 31, 2014, 2:55 p.m. | #5
On Fri, Oct 31, 2014 at 2:48 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
> On 10/30/2014 08:20 AM, Hendrik Leppkes wrote:
>> On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
>> wrote:
>>
>>> SPS_EXT can come before the PPS and results in incomplete extradata.
>>>
>> If you want this to be really complete, may also want to add SEI (6) and
>> subset SPS (15), I had some sort of crazy file where this was required to
>> get the SPS/PPS into extradata, but I lost it.
>>
>>
>
> Subset SPS is in a "reserved" range according to my copy of the h.264 spec (2005).  Has there been an update that adds
> this NAL?  If so, should we update h264.h with the new NAL code(s)?

It's one of the NALs added to support MVC and SVC.
I think it is correct that they are skipped since we don't support either.
Cheers,
Hendrik Leppkes Oct. 31, 2014, 4:52 p.m. | #6
On Fri, Oct 31, 2014 at 3:48 PM, John Stebbins <stebbins@jetheaddev.com>
wrote:

> On 10/30/2014 08:20 AM, Hendrik Leppkes wrote:
> > On Thu, Oct 30, 2014 at 4:05 PM, John Stebbins <stebbins@jetheaddev.com>
> > wrote:
> >
> >> SPS_EXT can come before the PPS and results in incomplete extradata.
> >>
> > If you want this to be really complete, may also want to add SEI (6) and
> > subset SPS (15), I had some sort of crazy file where this was required to
> > get the SPS/PPS into extradata, but I lost it.
> >
> >
>
> Subset SPS is in a "reserved" range according to my copy of the h.264 spec
> (2005).  Has there been an update that adds
> this NAL?  If so, should we update h264.h with the new NAL code(s)?
>
>
There is a 2012 version of the spec which includes it as a valid NAL.

- Hendrik

Patch

diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 145dce3..b9c47ab 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -474,7 +474,8 @@  static int h264_split(AVCodecContext *avctx,
          *  }
          */
         if ((state & 0xFFFFFF00) == 0x100 && (state & 0xFFFFFF1F) != 0x107 &&
-            (state & 0xFFFFFF1F) != 0x108 && (state & 0xFFFFFF1F) != 0x109) {
+            (state & 0xFFFFFF1F) != 0x108 && (state & 0xFFFFFF1F) != 0x109 &&
+            (state & 0xFFFFFF1F) != 0x10d) {
             if (has_sps) {
                 while (i > 4 && buf[i - 5] == 0)
                     i--;