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

[Andreas Håkon]

Hi Andreas,


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

I'm running a lot of tests with QSV. So I know a little bit about that.


> 1.
>
> > +                          if ((p_buf[7] & 0x1) == 1) {
> > +                              memcpy(q->matrix, p_buf + 8, 64);
> > +                              p_sec += 4;
> > +                              av_log(avctx, AV_LOG_VERBOSE, "Found
>
> 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.)

I'll revise it.

However, please note that in our tests the QSV MPEG-2 encoder has never
written an intra_quantiser_matrix. The code is here to complete it.


> 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?

As I say, the current driver never writes an intra_quantiser_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?

As far as I know, the QSV documentation does not describe anything about it.
Perhaps the best solution is to completely forget the intra_quantiser_matrix.
What do you think?


> 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.

Sure! But the bitstream from the QSV MPEG-2 encoder never generates
a sequence_scalable_extension nor a sequence_display_extension.
For this reason this patch only reinserts the SEQ_START_CODE and
EXT_START_CODE.


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

Well, the current implementation is pretty simple to understand.
I see no reason to change it, as it is an internal loop indicator.


> 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.)

Absolutly! It repeats only Group Of Pictures (01B8), Picture header (0100)
and Picture_Coding_Extension; and forgets the rest.
You can run more tests if you want. But the QSV MPEG-2 encoder has not been
updated for years on Intel drivers.


> 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.

Please, note that bs->Data is an already allocated buffer. The allocated
space is q->packet_size that correspond to:

- QSVEncContext *q;
- q->packet_size = q->param.mfx.BufferSizeInKB * 1000;

So the buffer is initialized to a large value inside the function
qsv_retrieve_enc_params() at the initilitation of the hw encoder.

Check it at:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/qsvenc.c#L860

So, then it's not a fault to throw the last bytes of the buffer, as the
Length is much small that the Size of the buffer.

Futhermore, the Length is the size of the data allocated inside the
buffer and not the size of the buffer. So, when you reinsert the missing
tables in the header you need to update the value of the Length.

That said, the code is correct.


> 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 }.

Yes, you're right.


More comments?
This problem really needs to be solved. The bitstream generated breaks the standard!

Regards.
A.H.

---