[FFmpeg-devel] [PATCH v2] avcodec: add an AV1 parser

[James Almer]

On 10/3/2018 8:26 PM, Mark Thompson wrote:
> On 03/10/18 02:44, James Almer wrote:
>> Simple parser to set keyframes, frame type, structure, width, height, and pixel
>> format, plus stream profile and level.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Missing Changelog entry and version bump still.
>>
>>  configure               |   1 +
>>  libavcodec/Makefile     |   1 +
>>  libavcodec/av1_parser.c | 226 ++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/parsers.c    |   1 +
>>  4 files changed, 229 insertions(+)
>>  create mode 100644 libavcodec/av1_parser.c
> 
> Looks good, and tested a bit (+1 for it removing the annoying need for -copyinkf when messing with BSFs in stream copy from raw IVF files).
> 
> One very minor nitpick below, then no more from me :)
> 
> Thanks,
> 
> - Mark
> 
> 
>> ...
>> diff --git a/libavcodec/av1_parser.c b/libavcodec/av1_parser.c
>> new file mode 100644
>> index 0000000000..0dee3e9d2e
>> --- /dev/null
>> +++ b/libavcodec/av1_parser.c
>> ...
>> +
>> +static int av1_parser_parse(AVCodecParserContext *ctx,
>> +                            AVCodecContext *avctx,
>> +                            const uint8_t **out_data, int *out_size,
>> +                            const uint8_t *data, int size)
>> +{
>> +    AV1ParseContext *s = ctx->priv_data;
>> +    CodedBitstreamFragment *td = &s->temporal_unit;
>> +    CodedBitstreamAV1Context *av1 = s->cbc->priv_data;
>> +    int ret;
>> +
>> +    *out_data = data;
>> +    *out_size = size;
>> +
>> +    ctx->key_frame         = -1;
>> +    ctx->pict_type         = AV_PICTURE_TYPE_NONE;
>> +    ctx->picture_structure = AV_PICTURE_STRUCTURE_UNKNOWN;
>> +
>> +    s->cbc->log_ctx = avctx;
> 
> I think this should be unset at the end of the function as well.  While I don't see a way for it to do so currently, some later call like the ff_cbs_close() could in theory attempt to use the logging context and then invoke undefined behaviour because the AVCodecContext assigned here need not still be accessible.

Yeah, good catch. Changed and pushed, thanks!

We may need to start planning on a replacement parser API. The fact it
still uses raw bitstreams instead of AVPackets is a real handicap. Just
by having to use ff_cbs_read() instead of ff_cbs_read_packet() this
parser is slower than it should, because of the data memcpy.