diff mbox

[FFmpeg-devel] avformat/avc: write the missing bits in the AVC Decoder Configuration Box

Message ID 20191126125721.105-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Nov. 26, 2019, 12:57 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/avc.c | 38 +++++++++++++++++++++++++++++++++-----
 libavformat/avc.h |  1 +
 2 files changed, 34 insertions(+), 5 deletions(-)

Comments

Andreas Rheinhardt Nov. 26, 2019, 1:57 p.m. UTC | #1 
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.)
Carl Eugen Hoyos Nov. 26, 2019, 5:32 p.m. UTC | #2 
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
James Almer Nov. 26, 2019, 8:04 p.m. UTC | #3 
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.
James Almer Nov. 27, 2019, 12:26 p.m. UTC | #4 
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.
James Almer Nov. 28, 2019, 6:24 p.m. UTC | #5 
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 mbox

Patch

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;