[PATCH 1/2] avcodec/h264: keep SPS and PPS bitstream data
Needed for the following VideotoolBox commit. --- libavcodec/h264.h | 4 ++++ libavcodec/h264_ps.c | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libavcodec/h264.h b/libavcodec/h264.h index 7356288..eeb2aaf 100644 --- a/libavcodec/h264.h +++ b/libavcodec/h264.h @@ -229,6 +229,8 @@ typedef struct SPS { int residual_color_transform_flag; ///< residual_colour_transform_flag int constraint_set_flags; ///< constraint_set[0-3]_flag int new; ///< flag to keep track if the decoder context needs re-init due to changed SPS + uint8_t data[1 << 16]; + int data_size; } SPS; /** @@ -254,6 +256,8 @@ typedef struct PPS { uint8_t scaling_matrix8[6][64]; uint8_t chroma_qp_table[2][QP_MAX_NUM+1]; ///< pre-scaled (with chroma_qp_index_offset) version of qp_table int chroma_qp_diff; + uint8_t data[1 << 16]; + int data_size; } PPS; /** diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 52d235c..fd16a95 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) int i, log2_max_frame_num_minus4; SPS *sps; + sps = av_mallocz(sizeof(SPS)); + if (!sps) + return AVERROR(ENOMEM); + + sps->data_size = h->gb.buffer_end - h->gb.buffer; + if (sps->data_size > sizeof(sps->data)) + goto fail; + memcpy(sps->data, h->gb.buffer, sps->data_size); + profile_idc = get_bits(&h->gb, 8); constraint_set_flags |= get_bits1(&h->gb) << 0; // constraint_set0_flag constraint_set_flags |= get_bits1(&h->gb) << 1; // constraint_set1_flag @@ -320,11 +329,8 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) if (sps_id >= MAX_SPS_COUNT) { av_log(h->avctx, AV_LOG_ERROR, "sps_id %u out of range\n", sps_id); - return AVERROR_INVALIDDATA; + goto fail; } - sps = av_mallocz(sizeof(SPS)); - if (!sps) - return AVERROR(ENOMEM); sps->sps_id = sps_id; sps->time_offset_length = 24; @@ -603,6 +609,12 @@ int ff_h264_decode_picture_parameter_set(H264Context *h, int bit_length) pps = av_mallocz(sizeof(PPS)); if (!pps) return AVERROR(ENOMEM); + pps->data_size = h->gb.buffer_end - h->gb.buffer; + if (pps->data_size > sizeof(pps->data)) { + ret = AVERROR_INVALIDDATA; + goto fail; + } + memcpy(pps->data, h->gb.buffer, pps->data_size); pps->sps_id = get_ue_golomb_31(&h->gb); if ((unsigned)pps->sps_id >= MAX_SPS_COUNT || !h->sps_buffers[pps->sps_id]) { -- 2.5.1
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly. Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream. The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; } +#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL; - /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; } -- 2.5.1
On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; }
+#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL;
- /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; }
This will still fail spectacularly with a SPS/PPS change mid-stream. I don't suppose it somehow accepts the SPS/PPS data in-band as well? - Hendrik
On Thu, 1 Oct 2015 18:29:00 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; }
+#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL;
- /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; }
This will still fail spectacularly with a SPS/PPS change mid-stream. I don't suppose it somehow accepts the SPS/PPS data in-band as well?
Well, it worked for the resolution change sample. It _should_ be using SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will reinitialize itself when they change (at least in most cases), and then it will also reinitialize this hwaccel, which means the code above is actually run in this situation. The h->sps and h->pps fields will be set to whatever is actually used by the decoder at this point. So I'm hoping that this change is actually pretty correct.
On Thu, Oct 1, 2015 at 6:39 PM, wm4 <nfxjfg@googlemail.com> wrote:
On Thu, 1 Oct 2015 18:29:00 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; }
+#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL;
- /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; }
This will still fail spectacularly with a SPS/PPS change mid-stream. I don't suppose it somehow accepts the SPS/PPS data in-band as well?
Well, it worked for the resolution change sample. It _should_ be using SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will reinitialize itself when they change (at least in most cases), and then it will also reinitialize this hwaccel, which means the code above is actually run in this situation. The h->sps and h->pps fields will be set to whatever is actually used by the decoder at this point. So I'm hoping that this change is actually pretty correct.
Is init re-called on resolution change alone, everything else the same? I guess it may actually work. Although there may still be other SPS/PPS changes that don't trigger it, so it seems a bit fragile. - Hendrik
On Thu, 1 Oct 2015 18:45:40 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:39 PM, wm4 <nfxjfg@googlemail.com> wrote:
On Thu, 1 Oct 2015 18:29:00 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; }
+#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL;
- /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; }
This will still fail spectacularly with a SPS/PPS change mid-stream. I don't suppose it somehow accepts the SPS/PPS data in-band as well?
Well, it worked for the resolution change sample. It _should_ be using SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will reinitialize itself when they change (at least in most cases), and then it will also reinitialize this hwaccel, which means the code above is actually run in this situation. The h->sps and h->pps fields will be set to whatever is actually used by the decoder at this point. So I'm hoping that this change is actually pretty correct.
Is init re-called on resolution change alone, everything else the same? I guess it may actually work. Although there may still be other SPS/PPS changes that don't trigger it, so it seems a bit fragile.
The code for this is in ff_h264_decode_slice_header() (look for needs_reinit). It actually detects whether the SPS changed at all (either if the ID of the referenced SPS changed, or if the referenced SPS was newly read). But then it tries to avoid reinitialization by checking whether specific fields changed. So it might not be 100% robust. Basically, the logic only respects what libavcodec's decoder needs. In theory, the hwaccel could export a flag that signals the need for full reinitialization on any change.
On Thu, Oct 1, 2015 at 9:54 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Thu, 1 Oct 2015 18:45:40 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:39 PM, wm4 <nfxjfg@googlemail.com> wrote:
On Thu, 1 Oct 2015 18:29:00 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_ 420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 9dec5fc..cc1e592 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame) return 0; }
+#define AV_W8(p, v) *(p) = (v) + CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx) { + H264Context *h = avctx->priv_data; CFDataRef data = NULL; + uint8_t *p; + int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size; + uint8_t *vt_extradata = av_malloc(vt_extradata_size); + if (!vt_extradata) + return NULL;
- /* Each VCL NAL in the bitstream sent to the decoder - * is preceded by a 4 bytes length header. - * Change the avcC atom header if needed, to signal headers of 4 bytes. */ - if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) { - uint8_t *rw_extradata = av_memdup(avctx->extradata, avctx->extradata_size); - - if (!rw_extradata) - return NULL; - - rw_extradata[4] |= 0x03; - - data = CFDataCreate(kCFAllocatorDefault, rw_extradata, avctx->extradata_size); - - av_freep(&rw_extradata); - } else { - data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, avctx->extradata_size); - } - + p = vt_extradata; + + AV_W8(p + 0, 1); /* version */ + AV_W8(p + 1, h->sps.data[0]); /* profile */ + AV_W8(p + 2, h->sps.data[1]); /* profile compat */ + AV_W8(p + 3, h->sps.data[2]); /* level */ + AV_W8(p + 4, 0xff); /* 6 bits reserved (111111) + 2 bits nal size length - 3 (11) */ + AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */ + AV_WB16(p + 6, h->sps.data_size + 1); + AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header + memcpy(p + 9, h->sps.data, h->sps.data_size); + p += 9 + h->sps.data_size; + AV_W8(p + 0, 1); /* number of pps */ + AV_WB16(p + 1, h->pps.data_size + 1); + AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header + memcpy(p + 4, h->pps.data, h->pps.data_size); + + p += 4 + h->pps.data_size; + av_assert0(p - vt_extradata == vt_extradata_size); + + data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size); + av_free(vt_extradata); return data; }
This will still fail spectacularly with a SPS/PPS change mid-stream. I don't suppose it somehow accepts the SPS/PPS data in-band as well?
Well, it worked for the resolution change sample. It _should_ be using SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will reinitialize itself when they change (at least in most cases), and then it will also reinitialize this hwaccel, which means the code above is actually run in this situation. The h->sps and h->pps fields will be set to whatever is actually used by the decoder at this point. So I'm hoping that this change is actually pretty correct.
Is init re-called on resolution change alone, everything else the same? I guess it may actually work. Although there may still be other SPS/PPS changes that don't trigger it, so it seems a bit fragile.
The code for this is in ff_h264_decode_slice_header() (look for needs_reinit). It actually detects whether the SPS changed at all (either if the ID of the referenced SPS changed, or if the referenced SPS was newly read). But then it tries to avoid reinitialization by checking whether specific fields changed. So it might not be 100% robust. Basically, the logic only respects what libavcodec's decoder needs.
In theory, the hwaccel could export a flag that signals the need for full reinitialization on any change.
I'm looking into this further, because h264 samples with mid-stream SPS/PPS changes currently fail to decode with the videotoolbox hwaccel. I see the `must_reinit` and `needs_reinit` code in h264_slice.c, which triggers reinitialization of the software decoder's internal state. However, I don't see how this would ever cause videotoolbox to get reinitialized (especially since AVVideotoolboxContext is usually created by the user up-front). I'm considering a patch to h264_videotoolbox's end_frame() which compares the previous SPS/PPS(s) with the current to see if they've changed so the VTDecompressionSession can be restarted. Alternatively, I could add a new `reinit` callback to AVHWAccel and invoke that. How do the other hwaccels handle mid-stream SPS changes? Aman
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta <ffmpeg@tmm1.net> wrote:
How do the other hwaccels handle mid-stream SPS changes?
Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS content for every frame, they don't keep a persistent state internally - that way the only "state" is the frame size and pixel format, and when those change get_format is called and the hwaccel re-initialized. - Hendrik
On Mon, 25 Sep 2017 09:02:36 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta <ffmpeg@tmm1.net> wrote:
How do the other hwaccels handle mid-stream SPS changes?
Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS content for every frame, they don't keep a persistent state internally - that way the only "state" is the frame size and pixel format, and when those change get_format is called and the hwaccel re-initialized.
Maybe it would be better if VT detected SPS/PPS changes itself, and reinitialized the VT session on demand when feeding slices. This way we wouldn't have to mess with the normal h264 software decoder reinit logic.
On Mon, Sep 25, 2017 at 3:06 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Mon, 25 Sep 2017 09:02:36 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta <ffmpeg@tmm1.net> wrote:
How do the other hwaccels handle mid-stream SPS changes?
Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS content for every frame, they don't keep a persistent state internally - that way the only "state" is the frame size and pixel format, and when those change get_format is called and the hwaccel re-initialized.
Maybe it would be better if VT detected SPS/PPS changes itself, and reinitialized the VT session on demand when feeding slices. This way we wouldn't have to mess with the normal h264 software decoder reinit logic.
Agreed. I'm trying to figure out the most efficient way to detect when the SPS/PPS changes. Previously I tried feeding in the SPS/PPS NALs from the decoder into the VT hwaccel (with a new `decode_params` callback), and using that to store a copy of the NAL in the hwaccel. I used a memcmp() against the previously stored value to detect when changes occurred and restarted the decompression session according. This approach worked well, but doesn't handle some streams which use multiple PPS. The most fool-proof way would be to construct a new avcC every time and only restart the session when it changes. But that seems quite expensive to be doing all the time. I'm also still not sure if the VT decoder needs to be restarted on SPS changes only, or PPS as well. Do new PPS usually accompany a new SPS? Is it also possible to have multiple SPS used at the same time? (The avcC construction code in videotoolbox.c assumes one SPS and one or more PPS). Aman
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, 25 Sep 2017 11:49:51 -0700 Aman Gupta <ffmpeg@tmm1.net> wrote:
On Mon, Sep 25, 2017 at 3:06 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Mon, 25 Sep 2017 09:02:36 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote:
On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta <ffmpeg@tmm1.net> wrote:
How do the other hwaccels handle mid-stream SPS changes?
Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS content for every frame, they don't keep a persistent state internally - that way the only "state" is the frame size and pixel format, and when those change get_format is called and the hwaccel re-initialized.
Maybe it would be better if VT detected SPS/PPS changes itself, and reinitialized the VT session on demand when feeding slices. This way we wouldn't have to mess with the normal h264 software decoder reinit logic.
Agreed. I'm trying to figure out the most efficient way to detect when the SPS/PPS changes.
Previously I tried feeding in the SPS/PPS NALs from the decoder into the VT hwaccel (with a new `decode_params` callback), and using that to store a copy of the NAL in the hwaccel. I used a memcmp() against the previously stored value to detect when changes occurred and restarted the decompression session according. This approach worked well, but doesn't handle some streams which use multiple PPS.
The most fool-proof way would be to construct a new avcC every time and only restart the session when it changes. But that seems quite expensive to be doing all the time.
I'm also still not sure if the VT decoder needs to be restarted on SPS changes only, or PPS as well. Do new PPS usually accompany a new SPS? Is it also possible to have multiple SPS used at the same time? (The avcC construction code in videotoolbox.c assumes one SPS and one or more PPS).
I don't think it's too expensive. Keep in mind that we copy around the actual slices 2 times as well, and the cost of rebuilding the avcc is probably the smallest part, and the memcmp would barely matter. Also I think what matters is whether VT sees the SPS/PPS slices at all. Didn't you have success with feeding them as part of the sample buffer (along with slice NALs)? I wonder if VT would be fine with the SPS and PPS repeated for every slice.
On Thu, 1 Oct 2015 18:13:21 +0200 wm4 <nfxjfg@googlemail.com> wrote:
This affects Annex B streams (such as demuxed from .ts and others). It also handles the format change in reinit-large_420_8-to-small_420_8.h264 correctly.
Instead of passing through the extradata, create it on the fly it from the currently active SPS and PPS. Since reconstructing the PPS and SPS NALs would be very complicated and verbose, we use the NALs as they originally appeared in the bitstream.
The code for writing the extradata is somewhat derived from libavformat/avc.c, but it's small and different enough that sharing it is not really worth it. --- Even though it requires changes in the general h264 decoder (previous patch), this solution is much cleaner and more robust than my patch from yesterday. --- libavcodec/videotoolbox.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
Both patches applied, with some requested amends (here, IRC) applied.
On Thu, Oct 01, 2015 at 06:13:20PM +0200, wm4 wrote:
Needed for the following VideotoolBox commit. --- libavcodec/h264.h | 4 ++++ libavcodec/h264_ps.c | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/libavcodec/h264.h b/libavcodec/h264.h index 7356288..eeb2aaf 100644 --- a/libavcodec/h264.h +++ b/libavcodec/h264.h @@ -229,6 +229,8 @@ typedef struct SPS { int residual_color_transform_flag; ///< residual_colour_transform_flag int constraint_set_flags; ///< constraint_set[0-3]_flag int new; ///< flag to keep track if the decoder context needs re-init due to changed SPS + uint8_t data[1 << 16]; + int data_size; } SPS;
/** @@ -254,6 +256,8 @@ typedef struct PPS { uint8_t scaling_matrix8[6][64]; uint8_t chroma_qp_table[2][QP_MAX_NUM+1]; ///< pre-scaled (with chroma_qp_index_offset) version of qp_table int chroma_qp_diff; + uint8_t data[1 << 16]; + int data_size; } PPS;
/** diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 52d235c..fd16a95 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) int i, log2_max_frame_num_minus4; SPS *sps;
+ sps = av_mallocz(sizeof(SPS)); + if (!sps) + return AVERROR(ENOMEM); + + sps->data_size = h->gb.buffer_end - h->gb.buffer;
the subtraction could overflow the 32bit int range leading to a truncated or negative data_size [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
On Thu, 1 Oct 2015 19:08:21 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
On Thu, Oct 01, 2015 at 06:13:20PM +0200, wm4 wrote:
Needed for the following VideotoolBox commit. --- libavcodec/h264.h | 4 ++++ libavcodec/h264_ps.c | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/libavcodec/h264.h b/libavcodec/h264.h index 7356288..eeb2aaf 100644 --- a/libavcodec/h264.h +++ b/libavcodec/h264.h @@ -229,6 +229,8 @@ typedef struct SPS { int residual_color_transform_flag; ///< residual_colour_transform_flag int constraint_set_flags; ///< constraint_set[0-3]_flag int new; ///< flag to keep track if the decoder context needs re-init due to changed SPS + uint8_t data[1 << 16]; + int data_size; } SPS;
/** @@ -254,6 +256,8 @@ typedef struct PPS { uint8_t scaling_matrix8[6][64]; uint8_t chroma_qp_table[2][QP_MAX_NUM+1]; ///< pre-scaled (with chroma_qp_index_offset) version of qp_table int chroma_qp_diff; + uint8_t data[1 << 16]; + int data_size; } PPS;
/** diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 52d235c..fd16a95 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) int i, log2_max_frame_num_minus4; SPS *sps;
+ sps = av_mallocz(sizeof(SPS)); + if (!sps) + return AVERROR(ENOMEM); + + sps->data_size = h->gb.buffer_end - h->gb.buffer;
the subtraction could overflow the 32bit int range leading to a truncated or negative data_size
[...]
This is impossible. The C type used for NALs is int.
On Thu, Oct 01, 2015 at 07:26:47PM +0200, wm4 wrote:
On Thu, 1 Oct 2015 19:08:21 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
On Thu, Oct 01, 2015 at 06:13:20PM +0200, wm4 wrote:
Needed for the following VideotoolBox commit. --- libavcodec/h264.h | 4 ++++ libavcodec/h264_ps.c | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/libavcodec/h264.h b/libavcodec/h264.h index 7356288..eeb2aaf 100644 --- a/libavcodec/h264.h +++ b/libavcodec/h264.h @@ -229,6 +229,8 @@ typedef struct SPS { int residual_color_transform_flag; ///< residual_colour_transform_flag int constraint_set_flags; ///< constraint_set[0-3]_flag int new; ///< flag to keep track if the decoder context needs re-init due to changed SPS + uint8_t data[1 << 16]; + int data_size; } SPS;
/** @@ -254,6 +256,8 @@ typedef struct PPS { uint8_t scaling_matrix8[6][64]; uint8_t chroma_qp_table[2][QP_MAX_NUM+1]; ///< pre-scaled (with chroma_qp_index_offset) version of qp_table int chroma_qp_diff; + uint8_t data[1 << 16]; + int data_size; } PPS;
/** diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 52d235c..fd16a95 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) int i, log2_max_frame_num_minus4; SPS *sps;
+ sps = av_mallocz(sizeof(SPS)); + if (!sps) + return AVERROR(ENOMEM); + + sps->data_size = h->gb.buffer_end - h->gb.buffer;
the subtraction could overflow the 32bit int range leading to a truncated or negative data_size
[...]
This is impossible. The C type used for NALs is int.
yes as long as all callers of this global function use such int based sizes to initialize the pointers if a single one is changed or one is added to initialize them from a int64_t then this could overflow. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
On Thu, 1 Oct 2015 19:56:41 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
On Thu, Oct 01, 2015 at 07:26:47PM +0200, wm4 wrote:
On Thu, 1 Oct 2015 19:08:21 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
On Thu, Oct 01, 2015 at 06:13:20PM +0200, wm4 wrote:
Needed for the following VideotoolBox commit. --- libavcodec/h264.h | 4 ++++ libavcodec/h264_ps.c | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/libavcodec/h264.h b/libavcodec/h264.h index 7356288..eeb2aaf 100644 --- a/libavcodec/h264.h +++ b/libavcodec/h264.h @@ -229,6 +229,8 @@ typedef struct SPS { int residual_color_transform_flag; ///< residual_colour_transform_flag int constraint_set_flags; ///< constraint_set[0-3]_flag int new; ///< flag to keep track if the decoder context needs re-init due to changed SPS + uint8_t data[1 << 16]; + int data_size; } SPS;
/** @@ -254,6 +256,8 @@ typedef struct PPS { uint8_t scaling_matrix8[6][64]; uint8_t chroma_qp_table[2][QP_MAX_NUM+1]; ///< pre-scaled (with chroma_qp_index_offset) version of qp_table int chroma_qp_diff; + uint8_t data[1 << 16]; + int data_size; } PPS;
/** diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 52d235c..fd16a95 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -307,6 +307,15 @@ int ff_h264_decode_seq_parameter_set(H264Context *h, int ignore_truncation) int i, log2_max_frame_num_minus4; SPS *sps;
+ sps = av_mallocz(sizeof(SPS)); + if (!sps) + return AVERROR(ENOMEM); + + sps->data_size = h->gb.buffer_end - h->gb.buffer;
the subtraction could overflow the 32bit int range leading to a truncated or negative data_size
[...]
This is impossible. The C type used for NALs is int.
yes as long as all callers of this global function use such int based sizes to initialize the pointers if a single one is changed or one is added to initialize them from a int64_t then this could overflow.
There could be much more wrong wtih this, like e.g. how the GetBits context is assumed to not have any bits written yet. But ok, I'll fix it on push by changing the data_size fields to size_t.
participants (4)
-
Aman Gupta -
Hendrik Leppkes -
Michael Niedermayer -
wm4