Message ID | 20191126125721.105-1-jamrial@gmail.com |
---|---|
State | New |
Headers | show |
On Tue, Nov 26, 2019 at 2:07 PM James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- > libavformat/avc.h | 1 + > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/libavformat/avc.c b/libavformat/avc.c > index a041e84357..9bd215c07f 100644 > --- a/libavformat/avc.c > +++ b/libavformat/avc.c > @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t > *buf_in, uint8_t **buf, int *size) > > int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) > { > - AVIOContext *sps_pb = NULL, *pps_pb = NULL; > + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; > uint8_t *buf = NULL, *end, *start = NULL; > - uint8_t *sps = NULL, *pps = NULL; > - uint32_t sps_size = 0, pps_size = 0; > - int ret, nb_sps = 0, nb_pps = 0; > + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; > + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; > + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; > > if (len <= 6) > return AVERROR_INVALIDDATA; > @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t > *data, int len) > if (ret < 0) > goto fail; > ret = avio_open_dyn_buf(&pps_pb); > + if (ret < 0) > + goto fail; > + ret = avio_open_dyn_buf(&sps_ext_pb); > if (ret < 0) > goto fail; > > @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const > uint8_t *data, int len) > } > avio_wb16(pps_pb, size); > avio_write(pps_pb, buf, size); > + } else if (nal_type == 13) { /* SPS_EXT */ > + nb_sps_ext++; > + if (size > UINT16_MAX || nb_sps_ext >= 256) { > + ret = AVERROR_INVALIDDATA; > + goto fail; > + } > + avio_wb16(sps_ext_pb, size); > + avio_write(sps_ext_pb, buf, size); > } > > buf += size; > } > sps_size = avio_close_dyn_buf(sps_pb, &sps); > pps_size = avio_close_dyn_buf(pps_pb, &pps); > + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); > > if (sps_size < 6 || !pps_size) { > ret = AVERROR_INVALIDDATA; > @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const > uint8_t *data, int len) > avio_w8(pb, nb_pps); /* number of pps */ > avio_write(pb, pps, pps_size); > > + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { > + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); > + if (!seq) > + goto fail; > + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved > (111111) + chroma_format_idc */ > + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved > (11111) + bit_depth_luma_minus8 */ > + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved > (11111) + bit_depth_chroma_minus8 */ > + avio_w8(pb, nb_sps_ext); /* number of sps ext */ > + if (nb_sps_ext) > + avio_write(pb, sps_ext, sps_ext_size); > + av_free(seq); > + } > + > fail: > if (!sps) > avio_close_dyn_buf(sps_pb, &sps); > if (!pps) > avio_close_dyn_buf(pps_pb, &pps); > + if (!sps_ext) > + avio_close_dyn_buf(sps_ext_pb, &sps_ext); > I don't like how these buffers are freed. How about the following approach: You don't close the dynamic buffer above; instead you just call avio_get_dyn_buf(). And here on fail you use ffio_free_dyn_buf() unconditionally instead of these combinations of avio_close_dyn_buf() and av_free(). (I have recently looked a bit into the dynamic buffers (mainly because the Matroska muxer benefits tremendously from setting direct for the dynamic buffer for the cluster) and there is a potential for improvements from which the above approach would benefit: One can modify avio_get_dyn_buf() to return a pointer to the small (1024 B) IO-buffer if nothing has been written to the big buffer yet; and if one also modifies ffio_free_dyn_buf to not call avio_close_dyn_buf(), but to free everything directly, an allocation could be saved if the data one intends to write fits into the IO-buffer.) I haven't looked at the rest. - Andreas PS: The SPS/PPS ids are supposed to be monotonically increasing, yet nothing in the above code ensures that. (This is not supposed to block your patch.)
Am Di., 26. Nov. 2019 um 14:07 Uhr schrieb James Almer <jamrial@gmail.com>: > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- > libavformat/avc.h | 1 + > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/libavformat/avc.c b/libavformat/avc.c > index a041e84357..9bd215c07f 100644 > --- a/libavformat/avc.c > +++ b/libavformat/avc.c > @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t *buf_in, uint8_t **buf, int *size) > > int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) > { > - AVIOContext *sps_pb = NULL, *pps_pb = NULL; > + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; > uint8_t *buf = NULL, *end, *start = NULL; > - uint8_t *sps = NULL, *pps = NULL; > - uint32_t sps_size = 0, pps_size = 0; > - int ret, nb_sps = 0, nb_pps = 0; > + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; > + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; > + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; > > if (len <= 6) > return AVERROR_INVALIDDATA; > @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) > if (ret < 0) > goto fail; > ret = avio_open_dyn_buf(&pps_pb); > + if (ret < 0) > + goto fail; > + ret = avio_open_dyn_buf(&sps_ext_pb); > if (ret < 0) > goto fail; > > @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) > } > avio_wb16(pps_pb, size); > avio_write(pps_pb, buf, size); > + } else if (nal_type == 13) { /* SPS_EXT */ > + nb_sps_ext++; > + if (size > UINT16_MAX || nb_sps_ext >= 256) { > + ret = AVERROR_INVALIDDATA; > + goto fail; > + } > + avio_wb16(sps_ext_pb, size); > + avio_write(sps_ext_pb, buf, size); > } > > buf += size; > } > sps_size = avio_close_dyn_buf(sps_pb, &sps); > pps_size = avio_close_dyn_buf(pps_pb, &pps); > + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); > > if (sps_size < 6 || !pps_size) { > ret = AVERROR_INVALIDDATA; > @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) > avio_w8(pb, nb_pps); /* number of pps */ > avio_write(pb, pps, pps_size); > > + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { > + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); > + if (!seq) > + goto fail; > + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved (111111) + chroma_format_idc */ > + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved (11111) + bit_depth_luma_minus8 */ > + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved (11111) + bit_depth_chroma_minus8 */ > + avio_w8(pb, nb_sps_ext); /* number of sps ext */ > + if (nb_sps_ext) > + avio_write(pb, sps_ext, sps_ext_size); > + av_free(seq); > + } > + > fail: > if (!sps) > avio_close_dyn_buf(sps_pb, &sps); > if (!pps) > avio_close_dyn_buf(pps_pb, &pps); > + if (!sps_ext) > + avio_close_dyn_buf(sps_ext_pb, &sps_ext); > av_free(sps); > av_free(pps); > + av_free(sps_ext); > av_free(start); > > return ret; > @@ -351,7 +379,7 @@ H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *buf, int buf_size) > skip_bits1(&gb); // separate_colour_plane_flag > } > sps->bit_depth_luma = get_ue_golomb(&gb) + 8; > - get_ue_golomb(&gb); // bit_depth_chroma_minus8 > + sps->bit_depth_chroma = get_ue_golomb(&gb) + 8; > skip_bits1(&gb); // qpprime_y_zero_transform_bypass_flag > if (get_bits1(&gb)) { // seq_scaling_matrix_present_flag > for (i = 0; i < ((sps->chroma_format_idc != 3) ? 8 : 12); i++) { > diff --git a/libavformat/avc.h b/libavformat/avc.h > index a79bf9b2db..5286d19d89 100644 > --- a/libavformat/avc.h > +++ b/libavformat/avc.h > @@ -43,6 +43,7 @@ typedef struct { > uint8_t constraint_set_flags; > uint8_t chroma_format_idc; > uint8_t bit_depth_luma; > + uint8_t bit_depth_chroma; > uint8_t frame_mbs_only_flag; > AVRational sar; > } H264SequenceParameterSet; Sorry if this is obvious: What does this patch fix? Carl Eugen
On 11/26/2019 2:32 PM, Carl Eugen Hoyos wrote: > Am Di., 26. Nov. 2019 um 14:07 Uhr schrieb James Almer <jamrial@gmail.com>: >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- >> libavformat/avc.h | 1 + >> 2 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/avc.c b/libavformat/avc.c >> index a041e84357..9bd215c07f 100644 >> --- a/libavformat/avc.c >> +++ b/libavformat/avc.c >> @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t *buf_in, uint8_t **buf, int *size) >> >> int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >> { >> - AVIOContext *sps_pb = NULL, *pps_pb = NULL; >> + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; >> uint8_t *buf = NULL, *end, *start = NULL; >> - uint8_t *sps = NULL, *pps = NULL; >> - uint32_t sps_size = 0, pps_size = 0; >> - int ret, nb_sps = 0, nb_pps = 0; >> + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; >> + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; >> + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; >> >> if (len <= 6) >> return AVERROR_INVALIDDATA; >> @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >> if (ret < 0) >> goto fail; >> ret = avio_open_dyn_buf(&pps_pb); >> + if (ret < 0) >> + goto fail; >> + ret = avio_open_dyn_buf(&sps_ext_pb); >> if (ret < 0) >> goto fail; >> >> @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >> } >> avio_wb16(pps_pb, size); >> avio_write(pps_pb, buf, size); >> + } else if (nal_type == 13) { /* SPS_EXT */ >> + nb_sps_ext++; >> + if (size > UINT16_MAX || nb_sps_ext >= 256) { >> + ret = AVERROR_INVALIDDATA; >> + goto fail; >> + } >> + avio_wb16(sps_ext_pb, size); >> + avio_write(sps_ext_pb, buf, size); >> } >> >> buf += size; >> } >> sps_size = avio_close_dyn_buf(sps_pb, &sps); >> pps_size = avio_close_dyn_buf(pps_pb, &pps); >> + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); >> >> if (sps_size < 6 || !pps_size) { >> ret = AVERROR_INVALIDDATA; >> @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >> avio_w8(pb, nb_pps); /* number of pps */ >> avio_write(pb, pps, pps_size); >> >> + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { >> + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); >> + if (!seq) >> + goto fail; >> + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved (111111) + chroma_format_idc */ >> + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved (11111) + bit_depth_luma_minus8 */ >> + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved (11111) + bit_depth_chroma_minus8 */ >> + avio_w8(pb, nb_sps_ext); /* number of sps ext */ >> + if (nb_sps_ext) >> + avio_write(pb, sps_ext, sps_ext_size); >> + av_free(seq); >> + } >> + >> fail: >> if (!sps) >> avio_close_dyn_buf(sps_pb, &sps); >> if (!pps) >> avio_close_dyn_buf(pps_pb, &pps); >> + if (!sps_ext) >> + avio_close_dyn_buf(sps_ext_pb, &sps_ext); >> av_free(sps); >> av_free(pps); >> + av_free(sps_ext); >> av_free(start); >> >> return ret; >> @@ -351,7 +379,7 @@ H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *buf, int buf_size) >> skip_bits1(&gb); // separate_colour_plane_flag >> } >> sps->bit_depth_luma = get_ue_golomb(&gb) + 8; >> - get_ue_golomb(&gb); // bit_depth_chroma_minus8 >> + sps->bit_depth_chroma = get_ue_golomb(&gb) + 8; >> skip_bits1(&gb); // qpprime_y_zero_transform_bypass_flag >> if (get_bits1(&gb)) { // seq_scaling_matrix_present_flag >> for (i = 0; i < ((sps->chroma_format_idc != 3) ? 8 : 12); i++) { >> diff --git a/libavformat/avc.h b/libavformat/avc.h >> index a79bf9b2db..5286d19d89 100644 >> --- a/libavformat/avc.h >> +++ b/libavformat/avc.h >> @@ -43,6 +43,7 @@ typedef struct { >> uint8_t constraint_set_flags; >> uint8_t chroma_format_idc; >> uint8_t bit_depth_luma; >> + uint8_t bit_depth_chroma; >> uint8_t frame_mbs_only_flag; >> AVRational sar; >> } H264SequenceParameterSet; > > Sorry if this is obvious: > What does this patch fix? > > Carl Eugen It adds the remaining portion of the avcC box as defined in the spec. Some parsers complain when the box ends abruptly, but most don't because they simply stop parsing after the pps part and skip the rest, so nobody noticed it.
On 11/26/2019 10:57 AM, Andreas Rheinhardt wrote: > On Tue, Nov 26, 2019 at 2:07 PM James Almer <jamrial@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- >> libavformat/avc.h | 1 + >> 2 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/avc.c b/libavformat/avc.c >> index a041e84357..9bd215c07f 100644 >> --- a/libavformat/avc.c >> +++ b/libavformat/avc.c >> @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t >> *buf_in, uint8_t **buf, int *size) >> >> int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >> { >> - AVIOContext *sps_pb = NULL, *pps_pb = NULL; >> + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; >> uint8_t *buf = NULL, *end, *start = NULL; >> - uint8_t *sps = NULL, *pps = NULL; >> - uint32_t sps_size = 0, pps_size = 0; >> - int ret, nb_sps = 0, nb_pps = 0; >> + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; >> + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; >> + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; >> >> if (len <= 6) >> return AVERROR_INVALIDDATA; >> @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t >> *data, int len) >> if (ret < 0) >> goto fail; >> ret = avio_open_dyn_buf(&pps_pb); >> + if (ret < 0) >> + goto fail; >> + ret = avio_open_dyn_buf(&sps_ext_pb); >> if (ret < 0) >> goto fail; >> >> @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const >> uint8_t *data, int len) >> } >> avio_wb16(pps_pb, size); >> avio_write(pps_pb, buf, size); >> + } else if (nal_type == 13) { /* SPS_EXT */ >> + nb_sps_ext++; >> + if (size > UINT16_MAX || nb_sps_ext >= 256) { >> + ret = AVERROR_INVALIDDATA; >> + goto fail; >> + } >> + avio_wb16(sps_ext_pb, size); >> + avio_write(sps_ext_pb, buf, size); >> } >> >> buf += size; >> } >> sps_size = avio_close_dyn_buf(sps_pb, &sps); >> pps_size = avio_close_dyn_buf(pps_pb, &pps); >> + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); >> >> if (sps_size < 6 || !pps_size) { >> ret = AVERROR_INVALIDDATA; >> @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const >> uint8_t *data, int len) >> avio_w8(pb, nb_pps); /* number of pps */ >> avio_write(pb, pps, pps_size); >> >> + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { >> + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); >> + if (!seq) >> + goto fail; >> + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved >> (111111) + chroma_format_idc */ >> + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved >> (11111) + bit_depth_luma_minus8 */ >> + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved >> (11111) + bit_depth_chroma_minus8 */ >> + avio_w8(pb, nb_sps_ext); /* number of sps ext */ >> + if (nb_sps_ext) >> + avio_write(pb, sps_ext, sps_ext_size); >> + av_free(seq); >> + } >> + >> fail: >> if (!sps) >> avio_close_dyn_buf(sps_pb, &sps); >> if (!pps) >> avio_close_dyn_buf(pps_pb, &pps); >> + if (!sps_ext) >> + avio_close_dyn_buf(sps_ext_pb, &sps_ext); >> > > I don't like how these buffers are freed. How about the following approach: > You don't close the dynamic buffer above; instead you just call > avio_get_dyn_buf(). And here on fail you use ffio_free_dyn_buf() > unconditionally instead of these combinations of avio_close_dyn_buf() and > av_free(). Ok, will change that and apply this soon.
On 11/27/2019 9:26 AM, James Almer wrote: > On 11/26/2019 10:57 AM, Andreas Rheinhardt wrote: >> On Tue, Nov 26, 2019 at 2:07 PM James Almer <jamrial@gmail.com> wrote: >> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- >>> libavformat/avc.h | 1 + >>> 2 files changed, 34 insertions(+), 5 deletions(-) >>> >>> diff --git a/libavformat/avc.c b/libavformat/avc.c >>> index a041e84357..9bd215c07f 100644 >>> --- a/libavformat/avc.c >>> +++ b/libavformat/avc.c >>> @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t >>> *buf_in, uint8_t **buf, int *size) >>> >>> int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) >>> { >>> - AVIOContext *sps_pb = NULL, *pps_pb = NULL; >>> + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; >>> uint8_t *buf = NULL, *end, *start = NULL; >>> - uint8_t *sps = NULL, *pps = NULL; >>> - uint32_t sps_size = 0, pps_size = 0; >>> - int ret, nb_sps = 0, nb_pps = 0; >>> + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; >>> + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; >>> + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; >>> >>> if (len <= 6) >>> return AVERROR_INVALIDDATA; >>> @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t >>> *data, int len) >>> if (ret < 0) >>> goto fail; >>> ret = avio_open_dyn_buf(&pps_pb); >>> + if (ret < 0) >>> + goto fail; >>> + ret = avio_open_dyn_buf(&sps_ext_pb); >>> if (ret < 0) >>> goto fail; >>> >>> @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const >>> uint8_t *data, int len) >>> } >>> avio_wb16(pps_pb, size); >>> avio_write(pps_pb, buf, size); >>> + } else if (nal_type == 13) { /* SPS_EXT */ >>> + nb_sps_ext++; >>> + if (size > UINT16_MAX || nb_sps_ext >= 256) { >>> + ret = AVERROR_INVALIDDATA; >>> + goto fail; >>> + } >>> + avio_wb16(sps_ext_pb, size); >>> + avio_write(sps_ext_pb, buf, size); >>> } >>> >>> buf += size; >>> } >>> sps_size = avio_close_dyn_buf(sps_pb, &sps); >>> pps_size = avio_close_dyn_buf(pps_pb, &pps); >>> + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); >>> >>> if (sps_size < 6 || !pps_size) { >>> ret = AVERROR_INVALIDDATA; >>> @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const >>> uint8_t *data, int len) >>> avio_w8(pb, nb_pps); /* number of pps */ >>> avio_write(pb, pps, pps_size); >>> >>> + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { >>> + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); >>> + if (!seq) >>> + goto fail; >>> + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved >>> (111111) + chroma_format_idc */ >>> + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved >>> (11111) + bit_depth_luma_minus8 */ >>> + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved >>> (11111) + bit_depth_chroma_minus8 */ >>> + avio_w8(pb, nb_sps_ext); /* number of sps ext */ >>> + if (nb_sps_ext) >>> + avio_write(pb, sps_ext, sps_ext_size); >>> + av_free(seq); >>> + } >>> + >>> fail: >>> if (!sps) >>> avio_close_dyn_buf(sps_pb, &sps); >>> if (!pps) >>> avio_close_dyn_buf(pps_pb, &pps); >>> + if (!sps_ext) >>> + avio_close_dyn_buf(sps_ext_pb, &sps_ext); >>> >> >> I don't like how these buffers are freed. How about the following approach: >> You don't close the dynamic buffer above; instead you just call >> avio_get_dyn_buf(). And here on fail you use ffio_free_dyn_buf() >> unconditionally instead of these combinations of avio_close_dyn_buf() and >> av_free(). > > Ok, will change that and apply this soon. Changed and applied.
diff --git a/libavformat/avc.c b/libavformat/avc.c index a041e84357..9bd215c07f 100644 --- a/libavformat/avc.c +++ b/libavformat/avc.c @@ -107,11 +107,11 @@ int ff_avc_parse_nal_units_buf(const uint8_t *buf_in, uint8_t **buf, int *size) int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) { - AVIOContext *sps_pb = NULL, *pps_pb = NULL; + AVIOContext *sps_pb = NULL, *pps_pb = NULL, *sps_ext_pb = NULL; uint8_t *buf = NULL, *end, *start = NULL; - uint8_t *sps = NULL, *pps = NULL; - uint32_t sps_size = 0, pps_size = 0; - int ret, nb_sps = 0, nb_pps = 0; + uint8_t *sps = NULL, *pps = NULL, *sps_ext = NULL; + uint32_t sps_size = 0, pps_size = 0, sps_ext_size = 0; + int ret, nb_sps = 0, nb_pps = 0, nb_sps_ext = 0; if (len <= 6) return AVERROR_INVALIDDATA; @@ -133,6 +133,9 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) if (ret < 0) goto fail; ret = avio_open_dyn_buf(&pps_pb); + if (ret < 0) + goto fail; + ret = avio_open_dyn_buf(&sps_ext_pb); if (ret < 0) goto fail; @@ -160,12 +163,21 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) } avio_wb16(pps_pb, size); avio_write(pps_pb, buf, size); + } else if (nal_type == 13) { /* SPS_EXT */ + nb_sps_ext++; + if (size > UINT16_MAX || nb_sps_ext >= 256) { + ret = AVERROR_INVALIDDATA; + goto fail; + } + avio_wb16(sps_ext_pb, size); + avio_write(sps_ext_pb, buf, size); } buf += size; } sps_size = avio_close_dyn_buf(sps_pb, &sps); pps_size = avio_close_dyn_buf(pps_pb, &pps); + sps_ext_size = avio_close_dyn_buf(sps_ext_pb, &sps_ext); if (sps_size < 6 || !pps_size) { ret = AVERROR_INVALIDDATA; @@ -183,13 +195,29 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len) avio_w8(pb, nb_pps); /* number of pps */ avio_write(pb, pps, pps_size); + if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) { + H264SequenceParameterSet *seq = ff_avc_decode_sps(sps, sps_size); + if (!seq) + goto fail; + avio_w8(pb, 0xfc | seq->chroma_format_idc); /* 6 bits reserved (111111) + chroma_format_idc */ + avio_w8(pb, 0xf8 | seq->bit_depth_luma - 8); /* 5 bits reserved (11111) + bit_depth_luma_minus8 */ + avio_w8(pb, 0xf8 | seq->bit_depth_chroma - 8); /* 5 bits reserved (11111) + bit_depth_chroma_minus8 */ + avio_w8(pb, nb_sps_ext); /* number of sps ext */ + if (nb_sps_ext) + avio_write(pb, sps_ext, sps_ext_size); + av_free(seq); + } + fail: if (!sps) avio_close_dyn_buf(sps_pb, &sps); if (!pps) avio_close_dyn_buf(pps_pb, &pps); + if (!sps_ext) + avio_close_dyn_buf(sps_ext_pb, &sps_ext); av_free(sps); av_free(pps); + av_free(sps_ext); av_free(start); return ret; @@ -351,7 +379,7 @@ H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *buf, int buf_size) skip_bits1(&gb); // separate_colour_plane_flag } sps->bit_depth_luma = get_ue_golomb(&gb) + 8; - get_ue_golomb(&gb); // bit_depth_chroma_minus8 + sps->bit_depth_chroma = get_ue_golomb(&gb) + 8; skip_bits1(&gb); // qpprime_y_zero_transform_bypass_flag if (get_bits1(&gb)) { // seq_scaling_matrix_present_flag for (i = 0; i < ((sps->chroma_format_idc != 3) ? 8 : 12); i++) { diff --git a/libavformat/avc.h b/libavformat/avc.h index a79bf9b2db..5286d19d89 100644 --- a/libavformat/avc.h +++ b/libavformat/avc.h @@ -43,6 +43,7 @@ typedef struct { uint8_t constraint_set_flags; uint8_t chroma_format_idc; uint8_t bit_depth_luma; + uint8_t bit_depth_chroma; uint8_t frame_mbs_only_flag; AVRational sar; } H264SequenceParameterSet;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++----- libavformat/avc.h | 1 + 2 files changed, 34 insertions(+), 5 deletions(-)