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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".