Message ID | hR-x0kS8dQ8NOL7zMQhvPXTu37nMQXs-bkkKvalVYI1krV6-w0lPSX71CSSiNQnEOzmbR-b1TIMCodjsZGOtuY_5guquDiTJxuIH3ZPXDmE=@protonmail.com |
---|---|
State | New |
Headers | show |
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. ---
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index a03ab69..18d4540 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -39,6 +39,7 @@ #include "qsv.h" #include "qsv_internal.h" #include "qsvenc.h" +#include "mpegvideo.h" static const struct { mfxU16 profile; @@ -998,6 +999,13 @@ int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q) int opaque_alloc = 0; int ret; + if (avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO) { + q->section_state = 0; + } + else { + q->section_state = -1; + } + q->param.AsyncDepth = q->async_depth; q->async_fifo = av_fifo_alloc(q->async_depth * qsv_fifo_item_size()); @@ -1296,6 +1304,10 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q, QSVFrame *qsv_frame = NULL; mfxEncodeCtrl* enc_ctrl = NULL; int ret; + int p_sec, p_bytes_left; + const uint8_t *p_buf; + const uint8_t *p_buf_end; + uint32_t start_code; if (frame) { ret = submit_frame(q, frame, &qsv_frame); @@ -1388,6 +1400,82 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q, 0 : ff_qsv_print_error(avctx, ret, "Error during encoding"); } + // Patch MPEG-2 missing Sequence Sections + if (avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO && + (bs->FrameType & MFX_FRAMETYPE_I || bs->FrameType & MFX_FRAMETYPE_xI)) { + if (q->section_state == 0) { + av_log(avctx, AV_LOG_VERBOSE, "Searching for MPEG-2 initial Sequence Sections\n"); + // LOAD Sequence sections + p_sec = 0; + p_buf = bs->Data; + p_buf_end = bs->Data + bs->DataLength; + while (p_buf < p_buf_end) { + start_code= -1; + p_buf = avpriv_find_start_code(p_buf, p_buf_end, &start_code); + p_bytes_left = p_buf_end - p_buf; + switch(start_code) { + case SEQ_START_CODE: + if (p_bytes_left >= 7) { + memcpy(q->seq_header, p_buf - 4, 13); + p_sec += 1; + av_log(avctx, AV_LOG_VERBOSE, "Found MPEG-2 SEQ_START_CODE Section\n"); + 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"); + } + } + continue; + case EXT_START_CODE: + if (p_bytes_left >= 5 && (p_buf[0] >> 4) == 0x01) { + memcpy(q->seq_ext, p_buf - 4, 10); + p_sec += 2; + av_log(avctx, AV_LOG_VERBOSE, "Found MPEG-2 EXT_START_CODE Section\n"); + } + continue; + case -1: + break; + default: + if (start_code >= SLICE_MIN_START_CODE && + start_code <= SLICE_MAX_START_CODE) + break; + continue; + } + break; + } + switch (p_sec) { + case 3: + q->section_state = 1; // SEQ_START_CODE & EXT_START_CODE + break; + case 7: + q->section_state = 2; // SEQ_START_CODE+Matrix & EXT_START_CODE + break; + default: + q->section_state = 0; // Insufficient data + } + } else { + // ADD stored sections + switch (q->section_state) { + 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; + default: + return ff_qsv_print_error(avctx, MFX_ERR_UNDEFINED_BEHAVIOR, + "Impossible to Patch MPEG-2 missing Sequence Sections"); + } + } + } + if (ret == MFX_WRN_INCOMPATIBLE_VIDEO_PARAM && frame->interlaced_frame) print_interlace_msg(avctx, q); diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index f2f4d38..f2c9fba 100644 --- a/libavcodec/qsvenc.h +++ b/libavcodec/qsvenc.h @@ -177,6 +177,11 @@ typedef struct QSVEncContext { int low_power; int gpb; + int section_state; // -1: not used; 0: uninitialized; 1: loaded without matrix; 2: loaded with matrix + uint8_t seq_header[13]; + uint8_t matrix[64]; + uint8_t seq_ext[10]; + int a53_cc; #if QSV_HAVE_MF