| Message ID | HP_IbVWZD8bOacQRNI6QLLfyWUGCNo6jnXurNxX8Zb5h_Si8aVOqvwUOgkjlZzehoeBbXVPq4a1cFzfa5J2Xt0fsE9AWdDsV8lbC_BtmYrg=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Andreas Håkon > Sent: Tuesday, May 28, 2019 6:48 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: [FFmpeg-devel] [PATCH] libavformat/qsvenc: repeat mpeg2 missing > headers [v2] > > Hi, > > This patch supersedes #13105 (https://patchwork.ffmpeg.org/patch/13105/) > > A new (simpler and more robust) implementationof of the reinsertion of the > missing headers in the MPEG-2 bitstream from the HW QSV encoder. > > The problem is quite simple: The bitstream generated by the MPEG-2 QSV > encoder only incorporates the SEQ_START_CODE and EXT_START_CODE > headers in the first GOP. This generates a result that is not suitable for > streaming or broadcasting. > With this patch the "mpeg2_qsv" encoder is at the same level as the > "mpeg2video", as the software implementation repeats these headers by > default in each GOP. > > Regards. > A.H. 1. Doesn't dump_extra filter to add extradata to all key packets work well? 2. You always insert such a repeat header without any option to turn it off, thus should not be default behavior IMHO.
On 28.05.2019, at 12:48, Andreas Håkon <andreas.hakon@protonmail.com> wrote: > Hi, > > This patch supersedes #13105 (https://patchwork.ffmpeg.org/patch/13105/) > > A new (simpler and more robust) implementationof of the reinsertion of the > missing headers in the MPEG-2 bitstream from the HW QSV encoder. > > The problem is quite simple: The bitstream generated by the MPEG-2 QSV > encoder only incorporates the SEQ_START_CODE and EXT_START_CODE > headers in the first GOP. This generates a result that is not suitable for > streaming or broadcasting. > With this patch the "mpeg2_qsv" encoder is at the same level as the > "mpeg2video", as the software implementation repeats these headers by default > in each GOP. I am sure I might be missing things, but I would think that this is only necessary when muxing into certain containers. I.e. I would think when muxing the stream into .mov, .mkv or .nut files your patch would only needlessly bloat the size. I admit this also applies to the native mpeg2 encoder, so this might be a design issue in FFmpeg, but it's the background why it feels a bit wrong to do it in the encoder... > > Regards. > A.H. > > --- > <0001-libavformat-qsvenc-repeat-mpeg2-missing-headers-v2.patch.txt> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 30 de May de 2019 20:48, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 28.05.2019, at 12:48, Andreas Håkonandreas.hakon@protonmail.com wrote: > > > Hi, > > This patch supersedes #13105 (https://patchwork.ffmpeg.org/patch/13105/) > > A new (simpler and more robust) implementationof of the reinsertion of the > > missing headers in the MPEG-2 bitstream from the HW QSV encoder. > > The problem is quite simple: The bitstream generated by the MPEG-2 QSV > > encoder only incorporates the SEQ_START_CODE and EXT_START_CODE > > headers in the first GOP. This generates a result that is not suitable for > > streaming or broadcasting. > > With this patch the "mpeg2_qsv" encoder is at the same level as the > > "mpeg2video", as the software implementation repeats these headers by default > > in each GOP. > > I am sure I might be missing things, but I would think that this is only necessary > when muxing into certain containers. > I.e. I would think when muxing the stream into .mov, .mkv or .nut files your patch > would only needlessly bloat the size. Yes and no. First of all, please let me know which MPEG-2 encoders do not repeat these headers when using MPEG-TS. For streaming repetion is absolutely required. And I feel that's the reason why the native mpeg2 encoder does it. > I admit this also applies to the native mpeg2 encoder, so this might be a design issue > in FFmpeg, but it's the background why it feels a bit wrong to do it in the encoder... Take note that the "dump_extra" filter with the parameter "remove" can be used to remove the repetition. But contrary to popular belief (IMHO), there is no way to repeat these headers using a bitstream filter. So for this reason this patch is necessary. In any case, if you suggest that this behaviour should be optional, then I can add a parameter to enable it and leave it disabled by default. You agree? Regards. A.H. ---
Andreas Håkon: > On Thursday, 30 de May de 2019 20:48, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: >> I admit this also applies to the native mpeg2 encoder, so this might be a design issue >> in FFmpeg, but it's the background why it feels a bit wrong to do it in the encoder... > > Take note that the "dump_extra" filter with the parameter "remove" can be used to remove > the repetition. But contrary to popular belief (IMHO), there is no way to repeat these > headers using a bitstream filter. So for this reason this patch is necessary. > dump_extra doesn't have a parameter named "remove" at all. You seem to confuse it with extract_extradata. - Andreas
Hi A. Rheinhardt , > > Take note that the "dump_extra" filter with the parameter "remove" can be used to remove > > the repetition. But contrary to popular belief (IMHO), there is no way to repeat these > > headers using a bitstream filter. So for this reason this patch is necessary. > > dump_extra doesn't have a parameter named "remove" at all. You seem to > confuse it with extract_extradata. Yes, sorry! You're right. I'm speaking about the use of the "extract_extradata" filter with the "remove" parameter to remove unneeded headers in other containerss. Thank you. Regards. A.H. ---
Hi, I need to admit that I'm completely wrong in the end. After spending a lot of time on a patch to solve the problem (making the MPEG-2 QSV encoder compatible with broadcasts), I discovered that this solution works: $ ffmpeg ... -c:v mpeg2_qsv -bsf:v 'dump_extra' -f mpegts ... Using this bitstream filter every GOP includes the missing sequence headers. However, one flaw persists: - The first GOP has duplicated SEQ_START_CODE and EXT_START_CODE headers. The problem is that the dump_extra filter re-injects the headers without **verifying** their existence. Until someone fixes this, I'm commenting it here for documentation. In addition, I hope that an example will be incorporated into the Documentation as a reference. In any case, thank you for pointing out the mistake! Regards. A.H. ---
On 04.06.2019, at 15:07, Andreas Håkon <andreas.hakon@protonmail.com> wrote: > Hi, > > I need to admit that I'm completely wrong in the end. After spending a lot > of time on a patch to solve the problem (making the MPEG-2 QSV encoder > compatible with broadcasts), I discovered that this solution works: > > $ ffmpeg ... -c:v mpeg2_qsv -bsf:v 'dump_extra' -f mpegts ... > > Using this bitstream filter every GOP includes the missing sequence headers. > However, one flaw persists: > > - The first GOP has duplicated SEQ_START_CODE and EXT_START_CODE headers. > > The problem is that the dump_extra filter re-injects the headers > without **verifying** their existence. > > Until someone fixes this, I'm commenting it here for documentation. > In addition, I hope that an example will be incorporated into the > Documentation as a reference. I think ideally MPEG-TS/ES/... muxers would detect such cases and print a warning suggesting this command, which I think would be much better than documentation. Maybe it could even detect relevant cases and insert it itself. But either way, ideally someone would spend some time considering what the right way is to design this.
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index 8dbad71..63ee198 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -859,6 +859,15 @@ static int qsv_retrieve_enc_params(AVCodecContext *avctx, QSVEncContext *q) q->packet_size = q->param.mfx.BufferSizeInKB * q->param.mfx.BRCParamMultiplier * 1000; + if (avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO) { + av_log(avctx, AV_LOG_DEBUG, "Reading MPEG-2 initial Sequence Sections (SPSBuffer:%d)\n", extradata.SPSBufSize); + q->add_headers = av_malloc(extradata.SPSBufSize); + if (!q->add_headers) + return AVERROR(ENOMEM); + q->add_headers_size = extradata.SPSBufSize; + memcpy(q->add_headers, extradata.SPSBuffer, q->add_headers_size); + } + if (!extradata.SPSBufSize || (need_pps && !extradata.PPSBufSize) #if QSV_HAVE_CO_VPS || (q->hevc_vps && !extradata_vps.VPSBufSize) @@ -999,6 +1008,7 @@ int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q) int ret; q->param.AsyncDepth = q->async_depth; + q->add_headers_size = 0; q->async_fifo = av_fifo_alloc(q->async_depth * qsv_fifo_item_size()); if (!q->async_fifo) @@ -1437,6 +1447,20 @@ int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q, ret = MFXVideoCORE_SyncOperation(q->session, *sync, 1000); } while (ret == MFX_WRN_IN_EXECUTION); + if (avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO && q->add_headers_size > 0 && + (bs->FrameType & MFX_FRAMETYPE_I || bs->FrameType & MFX_FRAMETYPE_IDR || bs->FrameType & MFX_FRAMETYPE_xI || bs->FrameType & MFX_FRAMETYPE_xIDR)) { + if (bs->Data[0] == 0x00 && bs->Data[1] == 0x00 && bs->Data[2] == 0x01 && bs->Data[3] != 0xb3 ) { + av_log(avctx, AV_LOG_DEBUG, "Missing MPEG-2 Sequence Sections, reinsertion required\n"); + if (q->add_headers_size + bs->DataLength <= q->packet_size) { + memmove(new_pkt.data + q->add_headers_size, new_pkt.data, bs->DataLength); + memcpy(new_pkt.data, q->add_headers, q->add_headers_size); + bs->DataLength += q->add_headers_size; + } else { + av_log(avctx, AV_LOG_WARNING, "Insufficient spacing to reinsert MPEG-2 Sequence Sections"); + } + } + } + new_pkt.dts = av_rescale_q(bs->DecodeTimeStamp, (AVRational){1, 90000}, avctx->time_base); new_pkt.pts = av_rescale_q(bs->TimeStamp, (AVRational){1, 90000}, avctx->time_base); new_pkt.size = bs->DataLength; @@ -1545,5 +1569,8 @@ int ff_qsv_enc_close(AVCodecContext *avctx, QSVEncContext *q) av_freep(&q->extparam); + if (q->add_headers_size > 0) + av_freep(&q->add_headers); + return 0; } diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index f2f4d38..ee81c51 100644 --- a/libavcodec/qsvenc.h +++ b/libavcodec/qsvenc.h @@ -177,6 +177,9 @@ typedef struct QSVEncContext { int low_power; int gpb; + int add_headers_size; + uint8_t *add_headers; + int a53_cc; #if QSV_HAVE_MF