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

[Andreas Rheinhardt]

Hello,

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?
>
Even if you only want to support the case you observed, it would IMO
simplify the code to use one big buffer for the sequence header (and
record the actual size of the sequence header) than use two small
buffers. That way you would not need to distinguish between the cases
1 and 2 when inserting. And if you use only one big buffer, it is easy
to support both matrices that may occur in a sequence header.

>> 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.
> 
It would not complicate anything to use |=.
> 
>> 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.
> 
No. If the situation is as you describe and only the first
bs->DataLength bytes of the buffer are valid, then you nevertheless
need to copy (memmove) all of the valid bytes, not only the first
bs->DataLength - 23/87.
The way your code above is written implies that bs->DataLength doesn't
need to be updated (as you throw away the last 23/87 valid bytes and
put 23/87 (valid) bytes at the front).

> More comments?

Yes. Why did you put all the new variables that you introduced into
such a large scope?
And there is the flag AV_CODEC_FLAG_GLOBAL_HEADER which if set means
that one should only put the headers into the extradata and not in
front of every keyframe. Maybe you could check for this before you
insert something?

> This problem really needs to be solved. The bitstream generated breaks the standard!
> 
If I am not mistaken, then the bitstream generated doesn't break the
standard; it is just inconvenient for streaming purposes.

- Andreas