[FFmpeg-devel] [PATCH v2 2/3] lavc: Add coded bitstream read/write support for AV1

[James Almer]

On 9/17/2018 8:47 PM, Mark Thompson wrote:
> ---
> On 10/09/18 00:40, James Almer wrote:
>> On 9/9/2018 7:08 PM, Mark Thompson wrote:
>>> +static int cbs_av1_split_fragment(CodedBitstreamContext *ctx,
>>> +                                  CodedBitstreamFragment *frag,
>>> +                                  int header)
>> ...
>> This is generating a lot of noise when using the trace_headers bsf.
>> Basically printing the header fields twice per OBU, first when
>> splitting, then again when decomposing.
>>
>> You can get rid of that and simplify this function a lot if you use the
>> ff_av1_packet_split() API from av1_parse.h doing more or less the same
>> to what you're doing for h2645:
> 
> I think I'm going to pass on using the split API here, since it doesn't have the same avoidance of overruns or the precise errors.  (Compared to H.26[45] there is a lot less going on here - we don't need to allocate anything, because there is no step to remove the emulation prevention bytes.)
> 
> So, I just fixed the output noise by switching off tracing inside that function.
> 
> 
> On 10/09/18 20:14, Michael Niedermayer wrote:
>> breaks build on mips:
>> CC	libavcodec/av1_metadata_bsf.o
>> In file included from src/libavcodec/av1_metadata_bsf.c:25:
>> src/libavcodec/cbs_av1.h:364: warning: declaration does not declare anything
>> src/libavcodec/cbs_av1.h:380: warning: declaration does not declare anything
>> ... lots of errors because of anonymous unions ...
> 
> Fixed by giving these unions redundant names.  Anonymous unions would be nice, but they're probably a little too much trouble still for old compilers.

I think GCC 4.8, used in CentOS, had trouble with those as well. But at
some point we should probably just set the minimum version to something
like GCC 4.9 (First version with C11 atomics). It would probably
simplify a lot of code beyond just anonymous unions.

> 
> 
> On 11/09/18 14:15, James Almer wrote:
>> On 9/9/2018 7:08 PM, Mark Thompson wrote:
>>> +static void cbs_av1_free_obu(void *unit, uint8_t *content)
>>> +{
>>> +    AV1RawOBU *obu = (AV1RawOBU*)content;
>>> +
>>> +    switch (obu->header.obu_type) {
>>> +    case AV1_OBU_TILE_GROUP:
>>> +        cbs_av1_free_tile_data(&obu->tile_group.tile_data);
>>> +        break;
>>> +    case AV1_OBU_FRAME:
>>> +        cbs_av1_free_tile_data(&obu->frame.tile_group.tile_data);
>>> +        break;
>>> +    case AV1_OBU_TILE_LIST:
>>> +        cbs_av1_free_tile_data(&obu->tile_list.tile_data);
>>> +        break;
>>> +    case AV1_OBU_METADATA:
>>> +        cbs_av1_free_metadata(&obu->metadata);
>>> +        break;
>>> +    }
>>> +
>>> +    av_freep(&obu);
>>
>> Why adding a custom free function for all OBUs when only four types need
>> it? Sounds like unnecessary overhead for all cases.
>> IMO what cbs_h2645 does is better, using the default free function where
>> a custom one isn't needed, then calling a custom free function directly
>> for those that need one.
> 
> I don't think I agree.  Adding separate free functions for each case requiring it would be more complex code overall (since the code making the OBU structures would need to select the right one rather than all using the same allocation as below), and the default free function is completely equivalent to this one in the other cases.
> 
> 
> Thanks,
> 
> - Mark

Looks good now. Thanks!