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

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
 doc/bitstream_filters.texi      |  5 +++++
 libavcodec/mpeg2_metadata_bsf.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Tom Yan:
> Signed-off-by: Tom Yan <tom.ty89@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).
>  
> +@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 }
>  };
>  
>

Tom Yan:
> Hi Andreas,
> 
> (Somehow I didn't receive your reply and only found it on the mailing
> list archive)
> 
> Can you be more specific about the "smaller scope" on the use of
> `MPEG2RawPictureCodingExtension`? Not sure what you mean.
> 

The actual scope of pce is the "if (ctx->ivtc) { }" block.

> The feature I'm trying to add mainly targets those soft telecined
> streams from NTSC "film" DVD which are actually entirely progressive
> (every single frame with progressive_frame and frame_pred_frame_dct
> being 1). In other words, what I'm targeting is supposed to be
> "progressive_sequence ready", but only faked as non-progressive
> sequence and have soft telecine applied (repeat_first_field and
> top_field_order).
> 
> On Wed, 30 Dec 2020 at 11:49, Tom Yan <tom.ty89@gmail.com> wrote:
>> +                    if (!pce->frame_pred_frame_dct) {
>> +                        if (pce->progressive_frame) {
>> +                            pce->frame_pred_frame_dct = 1;
> 
> I have it this way because I'm trying to be paranoid and lenient at
> the same time. From what I could see on the H.262 spec,
> progressive_sequence requires frame_pred_frame_dct. However, it
> doesn't exactly state so for progressive_frames. (From what I
> interpreted, progressive_frames "doesn't really matter" if
> progressive_sequence == 1.)
> 

The description of progressive sequence actually says that
progressive_frame needs to be 1 if progressive_sequence is 1:
"progressive_sequence – When set to '1' the coded video sequence
contains only progressive frame-pictures".

> What it states is, if progressive_frames == 1 and progressive_sequence
> == 1, frame_pred_frame_dct shall be 1. That's why I try to make use of
> progressive_frame as a "proof" to determine whether I can "fix"
> frame_pred_frame_dct, in case it somehow isn't 1.
> 

If frame_pred_frame_dct is zero, your input is not progressive sequence
ready (ok, there is a very slim chance that it is: If every frame only
uses frame prediction, you can patch the frames; but you need to parse
and patch the whole slices, not only the frame header, see the version I
referred to earlier [1]; I have not once found a DVD whose frames had
frame_pred_frame_dct unset that is progressive sequence ready, as there
were always frames with macroblocks that use interlaced prediction; but
there often are frames that only are compatible with
frame_pred_frame_dct despite it being unset and in this case one saves a
few bytes).
The reason that frame_pred_frame_dct can be zero even when
progressive_frame is set is that the standard allows bitstreams with a
mix of progressive and interlaced material. In this case one frame can
be progressive while its references are interlaced and so one can not
use progressive prediction (i.e. frame_pred_frame_dct is a "more global"
property).

> With that said I'm totally okay with removing this. I'm not sure
> what's the good way to bail out though. I mean, I cannot confirm the
> sequence is "progressive_sequence ready" until every frame is checked,
> and I cannot leave repeat_first_field and top_field_order untouched
> before I confirm so without looping twice. Yet how those two flags are
> interpreted depends heavily on progressive sequence. So I think the
> best thing I could do is, "blindly" set progressive_sequence and clear
> repeat_first_field and top_field_order, warn once (and maybe quit?) if
> any frame has frame_pred_frame_dct or progressive_frames == 0.
> 

Warning is not enough; you need to error out.

> The feature I'm adding isn't for making a bitstream progressive when
> it's not, but instead, for "unfaking" an entirely progressive
> bitstream, so I don't think the "modifying" bitstream / code height is
> relevant here.
> 
The coded height is one of the criteria for when a bitstream is actually
progressive. Notice that both 480 and 576 are divisible by 32, so this
is no problem for DVDs.

- Andreas

[1]:
https://github.com/mkver/FFmpeg/blob/4b5a477610962af29d155223d4044e8e55fd81b4/libavcodec/mpeg2_metadata_bsf.c