[FFmpeg-devel] [PATCH] mpeg2_metadata: support inverse (soft) telecine

[Andreas Rheinhardt]

Tom Yan:
> Signed-off-by: Tom Yan <tom.ty89 at gmail.com>
> ---
>  doc/bitstream_filters.texi      |  5 +++++
>  libavcodec/mpeg2_metadata_bsf.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
> index 8a2f55cc41..7831abc120 100644
> --- a/doc/bitstream_filters.texi
> +++ b/doc/bitstream_filters.texi
> @@ -499,6 +499,11 @@ table 6-6).
>  Set the colour description in the stream (see H.262 section 6.3.6
>  and tables 6-7, 6-8 and 6-9).
>  
> + at item ivtc
> +Inverse (soft) telecine, i.e. mark the sequences as progressive
> +and clear the repeat_first_field and top_field_first bits in
> +the Picture Coding Extension Data.
> +
>  @end table
>  
>  @section mpeg4_unpack_bframes
> diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c
> index d0048c0e25..735680a28b 100644
> --- a/libavcodec/mpeg2_metadata_bsf.c
> +++ b/libavcodec/mpeg2_metadata_bsf.c
> @@ -43,6 +43,8 @@ typedef struct MPEG2MetadataContext {
>      int transfer_characteristics;
>      int matrix_coefficients;
>  
> +    int ivtc;
> +
>      int mpeg1_warned;
>  } MPEG2MetadataContext;
>  
> @@ -54,7 +56,10 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf,
>      MPEG2RawSequenceHeader            *sh = NULL;
>      MPEG2RawSequenceExtension         *se = NULL;
>      MPEG2RawSequenceDisplayExtension *sde = NULL;
> +    MPEG2RawPictureCodingExtension   *pce = NULL;

This can have smaller scope.

>      int i, se_pos;
> +    int last_code = -1;
> +    int prog_seq = 1;
>  
>      for (i = 0; i < frag->nb_units; i++) {
>          if (frag->units[i].type == MPEG2_START_SEQUENCE_HEADER) {
> @@ -68,8 +73,26 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf,
>              } else if (ext->extension_start_code_identifier ==
>                  MPEG2_EXTENSION_SEQUENCE_DISPLAY) {
>                  sde = &ext->data.sequence_display;
> +            } else if (ext->extension_start_code_identifier ==
> +                MPEG2_EXTENSION_PICTURE_CODING && last_code ==
> +                MPEG2_START_PICTURE) {
> +                pce = &ext->data.picture_coding;
> +                if (ctx->ivtc) {
> +                    pce->repeat_first_field = 0;
> +                    pce->top_field_first = 0;
> +                    if (!pce->frame_pred_frame_dct) {
> +                        if (pce->progressive_frame) {
> +                            pce->frame_pred_frame_dct = 1;

This is completely wrong: frame_pred_frame_dct is used by the decoding
process and just setting it here to the value you want will result in
invalid output. You would have to modify the bitstream at the same time
you modify said flag; see
https://github.com/mkver/FFmpeg/blob/4b5a477610962af29d155223d4044e8e55fd81b4/libavcodec/mpeg2_metadata_bsf.c
for how to do it. The problem is: Not every bitstream is compatible with
the flag.

> +                        } else if (prog_seq) {
> +                            av_log(bsf, AV_LOG_ERROR, "applying ivtc "
> +                                   "to non-progressive sequence\n");
> +                            prog_seq = 0;
> +                        }
> +                    }
> +                }
>              }
>          }
> +        last_code = frag->units[i].type;
>      }
>  
>      if (!sh || !se) {
> @@ -167,6 +190,9 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf,
>          }
>      }
>  
> +    if (ctx->ivtc)
> +        se->progressive_sequence = 1;

progressive_sequence implies that all pictures need to be frames with
progressive_frame and frame_pred_frame_dct set. Yet you do not really
enforce this: Sometimes you will emit error messages (and in case you
emit one error message, you likely emit lots of error messages), but you
never error out. Furthermore, the coded height of a progressive sequence
is padded to 16, the coded height of a non-progressive sequence is
padded to 32. So if you switch from non-progressive sequence to
progressive sequence, you have to ensure that the coded height does not
change (in other words, this is only possible if the height padded to 16
is actually divisible by 32).

> +
>      return 0;
>  }
>  
> @@ -288,6 +314,10 @@ static const AVOption mpeg2_metadata_options[] = {
>          OFFSET(matrix_coefficients), AV_OPT_TYPE_INT,
>          { .i64 = -1 }, -1, 255, FLAGS },
>  
> +    { "ivtc", "Inverse (soft) Telecine",
> +        OFFSET(ivtc), AV_OPT_TYPE_BOOL,
> +        { .i64 = 0 }, 0, 1, FLAGS },
> +
>      { NULL }
>  };
>  
>