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

Hi,

A fix for the missing in-band Sequence Headers from the QSV MPEG-2 HW Encoder.

Regards.
A.H.

---
From a0b8525e0ebfd1a3b91bad7a21cc9de5c5a01e0e Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Tue, 14 May 2019 11:07:10 +0100
Subject: [PATCH] libavformat/qsvenc: fix mpeg2 missing headers

The current implementation of the QSV MPEG-2 HW encoder writes the value
of MPEG-2 sequence headers in-band one time only. That is, in the first GOP
of the stream. This behavior generates a bitstream that is not suitable for
streaming or broadcasting.
This patch resolves this problem by reading these headers the first time 
they appear, and reinserting them back into each GOP with the rest of
the MPEG-2 headers.

Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
---
 libavcodec/qsvenc.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/qsvenc.h |    5 +++
 2 files changed, 93 insertions(+)

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

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.

---

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

On Wed, May 15, 2019 at 09:39:00AM +0000, Andreas Rheinhardt wrote:
> > 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.

(sorry if it's nonsense, I did not investigate in detail)
If the problem to solve is to ensure headers are regularly
repeated, that does not sound like a QSV-specific problem
and more a job for a bitstream filter.
There might even be code like that already for the
case when we convert from formats with global headers
to formats like MPEG-TS that want them repeated.

Reimar Döffinger:
> On Wed, May 15, 2019 at 09:39:00AM +0000, Andreas Rheinhardt wrote:
>>> 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.
> 
> (sorry if it's nonsense, I did not investigate in detail)
> If the problem to solve is to ensure headers are regularly
> repeated, that does not sound like a QSV-specific problem
> and more a job for a bitstream filter.
> There might even be code like that already for the
> case when we convert from formats with global headers
> to formats like MPEG-TS that want them repeated.
There's actually a dump_extra bitstream filter.

- Andreas

Hi Andreas (Rheinhardt) and Reimar,

Thank you for your comments. You're right and the code needs
refactoring.
I prepared a new implementation, please review it:
https://patchwork.ffmpeg.org/patch/13316/

In any case, please note that the software encoder "mpeg2video"
every time writes the SEQ_START_CODE and EXT_START_CODE in
each GOP. So this isn't and optional functionality.

Futhermore, at least as far as I know, it's impossible to
reinsert these missing headers with the dump_extra bitstream filter.

Regards.
A.H.

---