On 21/05/17 09:46, Anton Khirnov wrote:
> Quoting Mark Thompson (2017-05-14 23:24:11)
>> ---
>> libavcodec/cbs.c | 1 +
>> libavcodec/cbs_h264.h | 396 +++++++++++++++
>> libavcodec/cbs_h2645.c | 734 ++++++++++++++++++++++++++++
>> libavcodec/cbs_h264_syntax.c | 1099
>> ++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/cbs_internal.h | 3 +
>> 5 files changed, 2233 insertions(+)
>> create mode 100644 libavcodec/cbs_h264.h
>> create mode 100644 libavcodec/cbs_h2645.c
>> create mode 100644 libavcodec/cbs_h264_syntax.c
>>
>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>> index 002c11cc6..93556eb91 100644
>> --- a/libavcodec/cbs.c
>> +++ b/libavcodec/cbs.c
>> @@ -27,6 +27,7 @@
>>
>>
>> static const CodedBitstreamType *cbs_type_table[] = {
>> + &ff_cbs_type_h264,
>> };
>>
>> int ff_cbs_init(CodedBitstreamContext *ctx,
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> new file mode 100644
>> index 000000000..8e8aae4da
>> --- /dev/null
>> +++ b/libavcodec/cbs_h264.h
>> @@ -0,0 +1,396 @@
>> +/*
>> + * This file is part of Libav.
>> + *
>> + * Libav is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * Libav is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with Libav; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
>> USA
>> + */
>> +
>> +#ifndef AVCODEC_CBS_H264_H
>> +#define AVCODEC_CBS_H264_H
>> +
>> +#include <stdint.h>
>> +
>> +
>> +enum {
>> + H264_MAX_SPS = 32,
>> + H264_MAX_PPS = 256,
>> +
>> + H264_MAX_REFS = 31,
>
> 31? You sure?
No. I'm off by one - it's num_ref_idx_lN_active*_minus1* that is constrained
to 0-31, so 32 references.
>> +
>> + H264_MAX_CPB_CNT = 32,
>> +
>> + H264_MAX_RPLM = H264_MAX_REFS + 1,
>> + H264_MAX_MMCO = H264_MAX_REFS * 2 + 3,
>> +
>> + H264_MAX_SEI_PAYLOADS = 64,
>
> Is this a spec-defined limit?
No - you can technically have as many as you like of some messages. This is
enough for one of every type and some more slack - only an insane stream could
hit this, and we do test for it and declare the stream stupid if there are more.
>> +};
>
> Would be nice to share those with the already existing values.
> (would be even nicer to share a lot of this code, but I understand
> that's a lot more work)
>
>> +static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>> + CodedBitstreamFragment *frag,
>> + const H2645Packet *packet)
>> +{
>> + int err, i, start;
>> +
>> + start = frag->nb_units;
>> + frag->nb_units += packet->nb_nals;
>> +
>> + err = av_reallocp_array(&frag->units, frag->nb_units,
>> + sizeof(*frag->units));
>
> Same as the comment for the other patch. Also, it seems rather weird and
> fragile that you're messing with the parent's internals directly
> rather than through a function call.
The ff_cbs_insert_unit() is meant to be the externalish API for adding blocks
which aren't internally managed - it adds the content only and doesn't do
anything with the coded bitstream data.
Hmm. A new internalish function to add a data-only block (which may be
decomposed later) would simplify this case, I'll go for that.
>> + if (err < 0)
>> + return err;
>> + memset(frag->units + start, 0,
>> + packet->nb_nals * sizeof(*frag->units));
>> +
>> + for (i = 0; i < packet->nb_nals; i++) {
>> + CodedBitstreamUnit *unit = &frag->units[start + i];
>> + const H2645NAL *nal = &packet->nals[i];
>> +
>> + unit->type = nal->type;
>> +
>> + unit->data = av_malloc(nal->size);
>> + if (!unit->data)
>> + return AVERROR(ENOMEM);
>> + memcpy(unit->data, nal->data, nal->size);
>> + unit->data_size = nal->size;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>> + CodedBitstreamFragment *frag,
>> + int header)
>> +{
>> + enum AVCodecID codec_id = ctx->codec->codec_id;
>> + H2645Packet packet = { 0 };
>> + int err;
>> +
>> + if (frag->nb_units != 0)
>> + return AVERROR(EINVAL);
>
> nit: I'm not a huge fan of such "is our caller being sane?" checks,
> since if the caller is insane then you're screwed either way and it can
> just hide bugs.
Sure. (It's originating from some case during development where the author was
insane, I think.)
>> +
>> + if (header && frag->data[0] && codec_id == AV_CODEC_ID_H264) {
>> + // AVCC header.
>> + CodedBitstreamH264Context *h264 = ctx->priv_data;
>> + uint8_t *data = frag->data;
>> + size_t size, start, end;
>> + int i, count;
>> +
>> + h264->avcc = 1;
>> + if (data[0] != 1) {
>> + av_log(ctx, AV_LOG_ERROR, "Invalid AVCC header: "
>> + "first byte %u.", *data);
>> + return AVERROR_INVALIDDATA;
>> + }
>> +
>> + h264->nal_length_size = (data[4] & 3) + 1;
>> + count = data[5] & 0x1f;
>
> Unchecked reads. I'd use the bytestream2 API.
I did that initially but it looked worse. I'll add a stronger check for the
size here.
>> +
>> + start = end = 6;
>> + for (i = 0; i < count; i++) {
>> + size = AV_RB16(data + end) + 2;
>> + if (end + size > frag->data_size)
>> + return AVERROR_INVALIDDATA;
>> + end += size;
>> + }
>> + err = ff_h2645_packet_split(&packet, frag->data + start, end -
>> start,
>> + ctx, 1, 2, AV_CODEC_ID_H264);
>> + if (err < 0) {
>> + av_log(ctx, AV_LOG_ERROR, "Failed to split AVCC SPSs.\n");
>> + return err;
>> + }
>> +
>> + err = cbs_h2645_fragment_add_nals(ctx, frag, &packet);
>> + ff_h2645_packet_uninit(&packet);
>
> You could store the packet in the context and call uninit only once at
> the end. It doesn't need to (and shouldn't) be called after every split.
Ah, right - you can keep splitting into the same packet structure and then add
all of the NAL units and uninit once at the end. I didn't notice that.
Yeah, that would be nicer. I don't see any reason why it shouldn't stay on the
stack, though.
>> +static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
>> + CodedBitstreamUnit *unit)
>> +{
>> + BitstreamContext bc;
>> + int err;
>> +
>> + err = bitstream_init(&bc, unit->data, 8 * unit->data_size);
>> + if (err < 0)
>> + return err;
>> +
>> + switch (unit->type) {
>> + case H264_NAL_SPS:
>> + {
>> + H264RawSPS *sps;
>> +
>> + sps = av_mallocz(sizeof(*sps));
>> + if (!sps)
>> + return AVERROR(ENOMEM);
>> + err = cbs_h264_read_sps(ctx, &bc, sps);
>> + if (err < 0) {
>> + av_free(sps);
>> + return err;
>> + }
>> +
>> + cbs_h264_replace_sps(ctx, sps);
>
> Should probably check the return value. Same below.
Yes.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
