[FFmpeg-devel] [PATCH] libavformat/qsvenc: fix mpeg2 missing headers

[Andreas Rheinhardt]

Hello,

Andreas Håkon:
> Hi,
> 
> A fix for the missing in-band Sequence Headers from the QSV MPEG-2 HW Encoder.
> 
> Regards.
> A.H.

I know nothing about QSV, but I know a bit about MPEG-2 and have
therefore taken a quick look at this:

1.
> +                        if ((p_buf[7] & 0x1) == 1) {
> +                            memcpy(q->matrix, p_buf + 8, 64);
> +                            p_sec += 4;
> +                            av_log(avctx, AV_LOG_VERBOSE, "Found
> MPEG-2 Matrix\n");
You are checking the wrong bit here (it should be the 0x2 bit) and you
are copying the wrong bits. That's because the intra_quantiser_matrix
matrix (if it is explicitly coded) doesn't start at a byte-aligned
position. Maybe you should not split the sequence header into two
buffers, but simply use one big buffer (and record the size of the
actual data in the buffer (which of course depends on the whether the
matrices are explicitly coded or not) somewhere)?
(If it is so that MPEG-2-QSV only ever uses the
non_intra_quantiser_matrix, then your approach might make sense.)

2. You are implicitly assuming that only one of the matrices exists,
but there can be two in the sequence header. Or is it documented
somewhere that MPEG-2-QSV can only use one matrix?

3. You are also implicitly assuming that there is no
quant_matrix_extension (which can have up to four matrices in it, but
only up to two for 4:2:0 video). Is it documented somewhere that this
is so?

4. According to the standard (section 6.1.1.6), if a sequence header
access unit contains a sequence_scalable_extension or
sequence_display_extension, then these need to be repeated in every
access unit with a repeat sequence header. So if MPEG-2-QSV might emit
them at the beginning, you need to record them and reinsert them along
with the rest every time you insert sequence headers.

5. You seem to use p_sec as a bitfield; so it would seem appropriate
to use |= to set the bits and not addition.

6. Is it really certain (and not only observed behaviour) that the QSV
encoder does not repeat sequence header in-band? (It is against the
specs to have two sequence headers in an access unit.)

7.
> +            case 1:
> +                memmove(bs->Data + 23, bs->Data, bs->DataLength - 23);
> +                bs->DataLength  += 23;
> +                memcpy( bs->Data + 0 , q->seq_header, 13);
> +                memcpy( bs->Data + 13, q->seq_ext,    10);
> +                break;
> +            case 2:
> +                memmove(bs->Data + 87, bs->Data, bs->DataLength - 87);
> +                bs->DataLength  += 87;
> +                memcpy( bs->Data + 0 , q->seq_header, 13);
> +                memcpy( bs->Data + 13, q->matrix,     64);
> +                memcpy( bs->Data + 77, q->seq_ext,    10);
> +                break;

This looks extremely fishy: You essentially throw the last 23/87 bytes
of the bs.Data buffer away and nevertheless you increase the length of
the data by 23/87 bytes.

8.
>+    if (avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO) {
>+        q->section_state = 0;
>+    }
>+    else {
>+        q->section_state = -1;
>+    }

The "else {" should be on the same line as the preceding closing }.

- Andreas