[FFmpeg-devel,5/7] cbs_mpeg2: Fix parsing the last unit

Submitted by Andreas Rheinhardt on July 29, 2019, 7:56 p.m.

Details

Message ID 20190729195658.56078-5-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt July 29, 2019, 7:56 p.m.
There is one way to find out if avpriv_find_start_code has found a start
code or not: One has to check whether the state variable contains a
start code, i.e. whether the three most serious bytes are 0x00 00 01.
Checking for whether the return value is the end of the designated
buffer is not enough: If the last four bytes constitute a start code,
the return value is also the end of the buffer. This happens with
sequence_end_codes which have been ignored for exactly this reason,
although e.g. all three files used for fate tests of cbs_mpeg2 contain
sequence_end_codes.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/cbs_mpeg2.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

Mark Thompson July 29, 2019, 10:21 p.m.
On 29/07/2019 20:56, Andreas Rheinhardt wrote:
> There is one way to find out if avpriv_find_start_code has found a start
> code or not: One has to check whether the state variable contains a
> start code, i.e. whether the three most serious bytes are 0x00 00 01.

"most significant bytes"

> Checking for whether the return value is the end of the designated
> buffer is not enough: If the last four bytes constitute a start code,
> the return value is also the end of the buffer. This happens with
> sequence_end_codes which have been ignored for exactly this reason,
> although e.g. all three files used for fate tests of cbs_mpeg2 contain
> sequence_end_codes.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/cbs_mpeg2.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index c529038fd9..596e9677b7 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -170,27 +170,41 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>      CodedBitstreamUnitType unit_type;
>      uint32_t start_code = -1;
>      size_t unit_size;
> -    int err, i;
> +    int err, i, final = 0;
>  
>      start = avpriv_find_start_code(frag->data, frag->data + frag->data_size,
>                                     &start_code);
> +    if (start_code >> 8 != 0x01) {
> +        // No start code found.
> +        return AVERROR_INVALIDDATA;
> +    }
> +
>      for (i = 0;; i++) {
>          unit_type = start_code & 0xff;
>  
> +        if (start == frag->data + frag->data_size) {
> +            // The last four bytes form a start code which constitutes
> +            // a unit of its own.  In this situation avpriv_find_start_code
> +            // won't modify start_code at all so modify start_code so that
> +            // the next unit will be treated as the last unit.
> +            start_code = 0;
> +        }
> +
>          end = avpriv_find_start_code(start--, frag->data + frag->data_size,
>                                       &start_code);
>  
>          // start points to the byte containing the start_code_identifier
> -        // (or to the last byte of fragment->data); end points to the byte
> +        // (may be the last byte of fragment->data); end points to the byte
>          // following the byte containing the start code identifier (or to
>          // the end of fragment->data).
> -        if (end == frag->data + frag->data_size) {
> -            // We didn't find a start code, so this is the final unit.
> -            unit_size = end - start;
> -        } else {
> +        if (start_code >> 8 == 0x01) {
>              // Unit runs from start to the beginning of the start code
>              // pointed to by end (including any padding zeroes).
>              unit_size = (end - 4) - start;
> +        } else {
> +           // We didn't find a start code, so this is the final unit.
> +           unit_size = end - start;
> +           final     = 1;
>          }
>  
>          err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, (uint8_t*)start,
> @@ -198,7 +212,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>          if (err < 0)
>              return err;
>  
> -        if (end == frag->data + frag->data_size)
> +        if (final)
>              break;
>  
>          start = end;
> 

1-5 LGTM, applied.

Thanks,

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
index c529038fd9..596e9677b7 100644
--- a/libavcodec/cbs_mpeg2.c
+++ b/libavcodec/cbs_mpeg2.c
@@ -170,27 +170,41 @@  static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
     CodedBitstreamUnitType unit_type;
     uint32_t start_code = -1;
     size_t unit_size;
-    int err, i;
+    int err, i, final = 0;
 
     start = avpriv_find_start_code(frag->data, frag->data + frag->data_size,
                                    &start_code);
+    if (start_code >> 8 != 0x01) {
+        // No start code found.
+        return AVERROR_INVALIDDATA;
+    }
+
     for (i = 0;; i++) {
         unit_type = start_code & 0xff;
 
+        if (start == frag->data + frag->data_size) {
+            // The last four bytes form a start code which constitutes
+            // a unit of its own.  In this situation avpriv_find_start_code
+            // won't modify start_code at all so modify start_code so that
+            // the next unit will be treated as the last unit.
+            start_code = 0;
+        }
+
         end = avpriv_find_start_code(start--, frag->data + frag->data_size,
                                      &start_code);
 
         // start points to the byte containing the start_code_identifier
-        // (or to the last byte of fragment->data); end points to the byte
+        // (may be the last byte of fragment->data); end points to the byte
         // following the byte containing the start code identifier (or to
         // the end of fragment->data).
-        if (end == frag->data + frag->data_size) {
-            // We didn't find a start code, so this is the final unit.
-            unit_size = end - start;
-        } else {
+        if (start_code >> 8 == 0x01) {
             // Unit runs from start to the beginning of the start code
             // pointed to by end (including any padding zeroes).
             unit_size = (end - 4) - start;
+        } else {
+           // We didn't find a start code, so this is the final unit.
+           unit_size = end - start;
+           final     = 1;
         }
 
         err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, (uint8_t*)start,
@@ -198,7 +212,7 @@  static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
         if (err < 0)
             return err;
 
-        if (end == frag->data + frag->data_size)
+        if (final)
             break;
 
         start = end;