Re: [FFmpeg-devel] [PATCH 2/6] lavf: APV demuxer

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Mark Thompson:
> On 21/04/2025 01:54, Michael Niedermayer wrote:
>> On Sat, Apr 19, 2025 at 08:07:00PM +0100, Mark Thompson wrote:
>>> Demuxes raw streams as defined in draft spec section 10.2.
>>> ---
>>>  libavformat/Makefile     |   1 +
>>>  libavformat/allformats.c |   1 +
>>>  libavformat/apvdec.c     | 245 +++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 247 insertions(+)
>>>  create mode 100644 libavformat/apvdec.c
>>>
>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>> index a94ac66e7e..ef96c2762e 100644
>>> --- a/libavformat/Makefile
>>> +++ b/libavformat/Makefile
>>> @@ -119,6 +119,7 @@ OBJS-$(CONFIG_APTX_DEMUXER)              += aptxdec.o
>>>  OBJS-$(CONFIG_APTX_MUXER)                += rawenc.o
>>>  OBJS-$(CONFIG_APTX_HD_DEMUXER)           += aptxdec.o
>>>  OBJS-$(CONFIG_APTX_HD_MUXER)             += rawenc.o
>>> +OBJS-$(CONFIG_APV_DEMUXER)               += apvdec.o
>>>  OBJS-$(CONFIG_AQTITLE_DEMUXER)           += aqtitledec.o subtitles.o
>>>  OBJS-$(CONFIG_ARGO_ASF_DEMUXER)          += argo_asf.o
>>>  OBJS-$(CONFIG_ARGO_ASF_MUXER)            += argo_asf.o
>>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>>> index 445f13f42a..90a4fe64ec 100644
>>> --- a/libavformat/allformats.c
>>> +++ b/libavformat/allformats.c
>>> @@ -72,6 +72,7 @@ extern const FFInputFormat  ff_aptx_demuxer;
>>>  extern const FFOutputFormat ff_aptx_muxer;
>>>  extern const FFInputFormat  ff_aptx_hd_demuxer;
>>>  extern const FFOutputFormat ff_aptx_hd_muxer;
>>> +extern const FFInputFormat  ff_apv_demuxer;
>>>  extern const FFInputFormat  ff_aqtitle_demuxer;
>>>  extern const FFInputFormat  ff_argo_asf_demuxer;
>>>  extern const FFOutputFormat ff_argo_asf_muxer;
>>> diff --git a/libavformat/apvdec.c b/libavformat/apvdec.c
>>> new file mode 100644
>>> index 0000000000..008cbef708
>>> --- /dev/null
>>> +++ b/libavformat/apvdec.c
>>> @@ -0,0 +1,245 @@
>>> +/*
>>> + * This file is part of FFmpeg.
>>> + *
>>> + * FFmpeg 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.
>>> + *
>>> + * FFmpeg 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 FFmpeg; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>> + */
>>> +
>>
>>> +#include "libavcodec/apv.h"
>>
>> missing file
> 
> I will reorder the cbs patch before this one.
> 
>>> +#include "libavcodec/get_bits.h"
>>> +#include "avformat.h"
>>> +#include "avio_internal.h"
>>> +#include "demux.h"
>>> +#include "internal.h"
>>> +
>>> +
>>> +#define APV_TAG MKBETAG('a', 'P', 'v', '1')
>>> +
>>> +typedef struct APVHeaderInfo {
>>> +    uint8_t  pbu_type;
>>> +    uint16_t group_id;
>>> +
>>> +    uint8_t  profile_idc;
>>> +    uint8_t  level_idc;
>>> +    uint8_t  band_idc;
>>> +
>>> +    uint32_t frame_width;
>>> +    uint32_t frame_height;
>>> +
>>> +    uint8_t  chroma_format_idc;
>>> +    uint8_t  bit_depth_minus8;
>>> +
>>> +    enum AVPixelFormat pixel_format;
>>> +} APVHeaderInfo;
>>> +
>>> +static const enum AVPixelFormat apv_format_table[5][5] = {
>>> +    { AV_PIX_FMT_GRAY8,    AV_PIX_FMT_GRAY10,     AV_PIX_FMT_GRAY12,     AV_PIX_FMT_GRAY14, AV_PIX_FMT_GRAY16 },
>>> +    { 0 }, // 4:2:0 is not valid.
>>> +    { AV_PIX_FMT_YUV422P,  AV_PIX_FMT_YUV422P10,  AV_PIX_FMT_YUV422P12,  AV_PIX_FMT_GRAY14, AV_PIX_FMT_YUV422P16 },
>>> +    { AV_PIX_FMT_YUV444P,  AV_PIX_FMT_YUV444P10,  AV_PIX_FMT_YUV444P12,  AV_PIX_FMT_GRAY14, AV_PIX_FMT_YUV444P16 },
>>> +    { AV_PIX_FMT_YUVA444P, AV_PIX_FMT_YUVA444P10, AV_PIX_FMT_YUVA444P12, AV_PIX_FMT_GRAY14, AV_PIX_FMT_YUVA444P16 },
>>> +};
>>> +
>>
>>> +static int apv_extract_header_info(APVHeaderInfo *info,
>>> +                                   GetBitContext *gbc)
>>> +{
>>> +    int zero, bit_depth_index;
>>> +
>>> +    info->pbu_type = get_bits(gbc, 8);
>>> +    info->group_id = get_bits(gbc, 16);
>>> +
>>> +    zero = get_bits(gbc, 8);
>>> +    if (zero != 0)
>>> +        return 0;
>>
>> this should use negative AVERROR* for errors for consistency
> 
> Ok.
> 
>>> +
>>> +    if (info->pbu_type != APV_PBU_PRIMARY_FRAME)
>>> +        return 0;
>>> +
>>> +    info->profile_idc = get_bits(gbc, 8);
>>> +    info->level_idc   = get_bits(gbc, 8);
>>> +    info->band_idc    = get_bits(gbc, 3);
>>> +
>>> +    zero = get_bits(gbc, 5);
>>> +    if (zero != 0)
>>> +        return 0;
>>> +
>>
>>> +    info->frame_width  = get_bits(gbc, 24);
>>> +    info->frame_height = get_bits(gbc, 24);
>>
>> frame_width and frame_height would be better as int instead of uint32_t
>> given its only 24bit
>>
>> its also more consistent
>> git grep 'int *width' | wc
>>    1935   15372  180444
>>
>> git grep 'uint32_t *width' | wc
>>      15      62     740
> 
> Sure.  We can change it to uint32_t later if there is a decision to use APVDecoderConfigurationRecord directly.
> 
>> [...]
>>> +static int apv_probe(const AVProbeData *p)
>>> +{
>>> +    GetBitContext gbc;
>>> +    APVHeaderInfo header;
>>> +    uint32_t au_size, tag, pbu_size;
>>> +    int score = AVPROBE_SCORE_EXTENSION + 1;
>>> +    int ret;
>>> +
>>> +    init_get_bits8(&gbc, p->buf, p->buf_size);
>>> +
>>> +    au_size = get_bits_long(&gbc, 32);
>>> +    if (au_size < 16) {
> 
> Here ^
> 
>>> +        // Too small.
>>> +        return 0;
>>> +    }
>>> +    // The spec doesn't have this tag, but the reference software and
>>> +    // all current files do.  Treat it as optional and skip if present,
>>> +    // but if it is there then this is definitely an APV file.
>>> +    tag = get_bits_long(&gbc, 32);
>>> +    if (tag == APV_TAG) {
>>> +        pbu_size = get_bits_long(&gbc, 32);
>>> +        score = AVPROBE_SCORE_MAX;
>>> +    } else {
>>> +        pbu_size = tag;
>>> +    }
>>> +    if (pbu_size < 16) {
>>> +        // Too small.
>>> +        return 0;
>>> +    }
>>> +
>>> +    ret = apv_extract_header_info(&header, &gbc);
>>
>> something somewhere should check buf_size or size in gbc
>> otherwise a truncated header would be accepted
> 
> See check above.
> 

He means a check for buf_size, not a check for the size field inside the
buffer.
Apart from that: The GetBit API expects its buffer to be
AV_INPUT_BUFFER_PADDING_SIZE bytes larger than what is used in
init_get_bits8(). (That is overcautious and way more than the current
implementation needs, but it is documented that way.) IMO the best way
is to switch to the bytestream API, as reading bitwise can be easily
avoided.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".