[FFmpeg-devel,v2] avcodec/h264_sei: Add acces to truncated SEI data

Submitted by Antonin Gouzer on June 8, 2019, 1:21 p.m.

Details

Message ID 20190608132145.10567-1-antonin.gouzer@gmail.com
State New
Headers show

Commit Message

Antonin Gouzer June 8, 2019, 1:21 p.m.
---
Some codecs editors had miss interpreted the H264 standart and
have coded a wrong size in the SEI data.
size = SEI size + 1.
The SEI data is detected as "truncated"
Ex: https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing
Command:
ffprobe -print_format xml -show_frames -read_intervals %+0.04 truncated.h264 
This (simple) patch add the possibility to read this false truncated SEI data with the default stric_std_compliance or less.
The error remain logged in both cases.

V2: Modifiy the patch for only the off by one values

Thanks in advance !
---
 libavcodec/h264_sei.c | 24 +++++++++++++++---------
 libavcodec/h264_sei.h |  2 +-
 2 files changed, 16 insertions(+), 10 deletions(-)

Comments

Reimar Döffinger June 8, 2019, 4:09 p.m.
looks good to me if maintainer has no objections...

On 08.06.2019, at 15:21, Antonin Gouzer <antonin.gouzer@gmail.com> wrote:

> ---
> Some codecs editors had miss interpreted the H264 standart and
> have coded a wrong size in the SEI data.
> size = SEI size + 1.
> The SEI data is detected as "truncated"
> Ex: https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing
> Command:
> ffprobe -print_format xml -show_frames -read_intervals %+0.04 truncated.h264 
> This (simple) patch add the possibility to read this false truncated SEI data with the default stric_std_compliance or less.
> The error remain logged in both cases.
> 
> V2: Modifiy the patch for only the off by one values
> 
> Thanks in advance !
> ---
> libavcodec/h264_sei.c | 24 +++++++++++++++---------
> libavcodec/h264_sei.h |  2 +-
> 2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index d4eb9c0dab..7871cf87ed 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -402,7 +402,7 @@ static int decode_alternative_transfer(H264SEIAlternativeTransfer *h,
> }
> 
> int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> -                       const H264ParamSets *ps, void *logctx)
> +                       const H264ParamSets *ps, AVCodecContext *avctx)
> {
>     int master_ret = 0;
> 
> @@ -425,27 +425,33 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>         } while (get_bits(gb, 8) == 255);
> 
>         if (size > get_bits_left(gb) / 8) {
> -            av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n",
> +            
> +            if (size == get_bits_left(gb) / 8 + 1 && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL){
> +                av_log(avctx, AV_LOG_WARNING, "SEI type %d size %d truncated at %d\n",
>                    type, 8*size, get_bits_left(gb));
> -            return AVERROR_INVALIDDATA;
> +               } else {
> +                av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d, data will not be read\n",
> +                   type, 8*size, get_bits_left(gb));
> +                return AVERROR_INVALIDDATA;
> +               }
>         }
>         next = get_bits_count(gb) + 8 * size;
> 
>         switch (type) {
>         case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
> -            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
> +            ret = decode_picture_timing(&h->picture_timing, gb, ps, avctx);
>             break;
>         case H264_SEI_TYPE_USER_DATA_REGISTERED:
> -            ret = decode_registered_user_data(h, gb, logctx, size);
> +            ret = decode_registered_user_data(h, gb, avctx, size);
>             break;
>         case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
> -            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
> +            ret = decode_unregistered_user_data(&h->unregistered, gb, avctx, size);
>             break;
>         case H264_SEI_TYPE_RECOVERY_POINT:
> -            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
> +            ret = decode_recovery_point(&h->recovery_point, gb, avctx);
>             break;
>         case H264_SEI_TYPE_BUFFERING_PERIOD:
> -            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
> +            ret = decode_buffering_period(&h->buffering_period, gb, ps, avctx);
>             break;
>         case H264_SEI_TYPE_FRAME_PACKING:
>             ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
> @@ -460,7 +466,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>             ret = decode_alternative_transfer(&h->alternative_transfer, gb);
>             break;
>         default:
> -            av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
> +            av_log(avctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
>         }
>         if (ret < 0 && ret != AVERROR_PS_NOT_FOUND)
>             return ret;
> diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h
> index a75c3aa175..32f3d67096 100644
> --- a/libavcodec/h264_sei.h
> +++ b/libavcodec/h264_sei.h
> @@ -190,7 +190,7 @@ typedef struct H264SEIContext {
> struct H264ParamSets;
> 
> int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> -                       const struct H264ParamSets *ps, void *logctx);
> +                       const struct H264ParamSets *ps, AVCodecContext *avctx);
> 
> /**
>  * Reset SEI values at the beginning of the frame.
> -- 
> 2.11.0
> 
> _______________________________________________
> 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".
Michael Niedermayer June 8, 2019, 9 p.m.
On Sat, Jun 08, 2019 at 03:21:45PM +0200, Antonin Gouzer wrote:
> ---
> Some codecs editors had miss interpreted the H264 standart and
> have coded a wrong size in the SEI data.
> size = SEI size + 1.
> The SEI data is detected as "truncated"

This information does not end in the commit message, the result is a
commit message thats a bit too terse, information wise.


> Ex: https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing

do you want to submit a fate test ?


> Command:
> ffprobe -print_format xml -show_frames -read_intervals %+0.04 truncated.h264 
> This (simple) patch add the possibility to read this false truncated SEI data with the default stric_std_compliance or less.
> The error remain logged in both cases.
> 
> V2: Modifiy the patch for only the off by one values
> 
> Thanks in advance !
> ---
>  libavcodec/h264_sei.c | 24 +++++++++++++++---------
>  libavcodec/h264_sei.h |  2 +-
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index d4eb9c0dab..7871cf87ed 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -402,7 +402,7 @@ static int decode_alternative_transfer(H264SEIAlternativeTransfer *h,
>  }
>  
>  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> -                       const H264ParamSets *ps, void *logctx)
> +                       const H264ParamSets *ps, AVCodecContext *avctx)
>  {

you could split this change in a seperate patch, this would make git log
slightly more readable 


>      int master_ret = 0;
>  
> @@ -425,27 +425,33 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>          } while (get_bits(gb, 8) == 255);
>  
>          if (size > get_bits_left(gb) / 8) {
> -            av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n",
> +            
> +            if (size == get_bits_left(gb) / 8 + 1 && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL){
> +                av_log(avctx, AV_LOG_WARNING, "SEI type %d size %d truncated at %d\n",
>                     type, 8*size, get_bits_left(gb));
> +               } else {
> +                av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d, data will not be read\n",
> +                   type, 8*size, get_bits_left(gb));
> +                return AVERROR_INVALIDDATA;
> +               }

something is not right with the indention here

thx

[...]
Vittorio Giovara June 10, 2019, 4:52 a.m.
On Sat, Jun 8, 2019 at 9:28 AM Antonin Gouzer <antonin.gouzer@gmail.com>
wrote:

> ---
> Some codecs editors had miss interpreted the H264 standart and
> have coded a wrong size in the SEI data.
> size = SEI size + 1.
> The SEI data is detected as "truncated"
> Ex:
> https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing
> Command:
> ffprobe -print_format xml -show_frames -read_intervals %+0.04
> truncated.h264
> This (simple) patch add the possibility to read this false truncated SEI
> data with the default stric_std_compliance or less.
> The error remain logged in both cases.
>
> V2: Modifiy the patch for only the off by one values
>
> Thanks in advance !
> ---
>  libavcodec/h264_sei.c | 24 +++++++++++++++---------
>  libavcodec/h264_sei.h |  2 +-
>  2 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index d4eb9c0dab..7871cf87ed 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -402,7 +402,7 @@ static int
> decode_alternative_transfer(H264SEIAlternativeTransfer *h,
>  }
>
>  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> -                       const H264ParamSets *ps, void *logctx)
> +                       const H264ParamSets *ps, AVCodecContext *avctx)
>  {
>      int master_ret = 0;
>
>
This may be a minor note, but i don't think it's worth to check for std
compliance within a *parsing* function: in my opinion it would make more
sense to check in the calling function, and always allow for truncated sei,
and error out only if (avctx->error | AV_ERROR_EXPLODE) is set (sorry going
by memory here)

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index d4eb9c0dab..7871cf87ed 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -402,7 +402,7 @@  static int decode_alternative_transfer(H264SEIAlternativeTransfer *h,
 }
 
 int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
-                       const H264ParamSets *ps, void *logctx)
+                       const H264ParamSets *ps, AVCodecContext *avctx)
 {
     int master_ret = 0;
 
@@ -425,27 +425,33 @@  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
         } while (get_bits(gb, 8) == 255);
 
         if (size > get_bits_left(gb) / 8) {
-            av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n",
+            
+            if (size == get_bits_left(gb) / 8 + 1 && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL){
+                av_log(avctx, AV_LOG_WARNING, "SEI type %d size %d truncated at %d\n",
                    type, 8*size, get_bits_left(gb));
-            return AVERROR_INVALIDDATA;
+               } else {
+                av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d, data will not be read\n",
+                   type, 8*size, get_bits_left(gb));
+                return AVERROR_INVALIDDATA;
+               }
         }
         next = get_bits_count(gb) + 8 * size;
 
         switch (type) {
         case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
-            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
+            ret = decode_picture_timing(&h->picture_timing, gb, ps, avctx);
             break;
         case H264_SEI_TYPE_USER_DATA_REGISTERED:
-            ret = decode_registered_user_data(h, gb, logctx, size);
+            ret = decode_registered_user_data(h, gb, avctx, size);
             break;
         case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
-            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
+            ret = decode_unregistered_user_data(&h->unregistered, gb, avctx, size);
             break;
         case H264_SEI_TYPE_RECOVERY_POINT:
-            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
+            ret = decode_recovery_point(&h->recovery_point, gb, avctx);
             break;
         case H264_SEI_TYPE_BUFFERING_PERIOD:
-            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
+            ret = decode_buffering_period(&h->buffering_period, gb, ps, avctx);
             break;
         case H264_SEI_TYPE_FRAME_PACKING:
             ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
@@ -460,7 +466,7 @@  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
             ret = decode_alternative_transfer(&h->alternative_transfer, gb);
             break;
         default:
-            av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
+            av_log(avctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
         }
         if (ret < 0 && ret != AVERROR_PS_NOT_FOUND)
             return ret;
diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h
index a75c3aa175..32f3d67096 100644
--- a/libavcodec/h264_sei.h
+++ b/libavcodec/h264_sei.h
@@ -190,7 +190,7 @@  typedef struct H264SEIContext {
 struct H264ParamSets;
 
 int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
-                       const struct H264ParamSets *ps, void *logctx);
+                       const struct H264ParamSets *ps, AVCodecContext *avctx);
 
 /**
  * Reset SEI values at the beginning of the frame.