[FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: add hevc support

[James Almer]

On 9/26/2017 11:49 PM, Aman Gupta wrote:
> On Tue, Sep 26, 2017 at 7:20 PM, James Almer <jamrial at gmail.com> wrote:
>> On 9/26/2017 10:08 PM, Aman Gupta wrote:
>>> +    for (i = 0; i < MAX_PPS_COUNT; i++) {
>>> +        if (h->ps.pps_list[i]) {
>>
>> I think this hints that there's no guarantee that the entire buffer of
>> size vt_extradata_size will be filled with data.
>> Keeping that in mind, you should use av_mallocz() for vt_extradata.
>>
>> Also, why did you use MAX_PPS_COUNT here but not to set vt_extradata_size?
>>
> 
> Hmm, I used MAX_PPS_COUNT in both loops (they should be identical as I
> copy/pasted), and the calculated size is exact based on the size of the *PS
> data. This is verified with the assert() at the end of this function. The
> approach was copied wholesale from avcc_create().

You're right, i missed the first loop. Nevermind this part then. I
missed it and assumed the buffer was allocated with the maximum size it
could in theory be needed.

> 
> 
>>
>>
>>> +            const HEVCPPS *pps = (const HEVCPPS
>> *)h->ps.pps_list[i]->data;
>>> +            AV_W8(p + 0, pps->data_size >> 8);
>>> +            AV_W8(p + 1, pps->data_size & 0xffff);
>>> +            memcpy(p + 2, pps->data, pps->data_size);
>>> +            p += 2 + pps->data_size;
>>> +        }
>>> +    }
>>> +
>>> +    av_assert0(p - vt_extradata == vt_extradata_size);
>>
>> This and all the pointer handling you did above makes me think the best
>> solution would be to use the bytestream2's PutByteContext API instead.
>>
>>> +
>>> +    data = CFDataCreate(kCFAllocatorDefault, vt_extradata,
>> vt_extradata_size);
>>
>> As i mentioned above, i don't think the entire buffer is guaranteed to
>> be always filled to the last byte. The bytestream2 API would let you
>> keep track of how much you wrote to it and pass that to this function.
>>
> 
> I will check out bytestream2 and see if it simplifies things. I'm using an
> extra loop to pre-calculate the size of the hvcC that I might be able to
> get rid of with the bytestream2 API.

You need to know the size of the allocated buffer (or the size you
intend to fill at most) to initialize the bytestream2 struct, so the
first loop will probably still be needed.

> 
> One advantage of the current approach is that it mimics the code in
> libavformat/hevc.c's hvcc_write(). This will make it easier to keep them in
> sync in the future.

lavf's hevc.c uses the AVIOContext API rather than intreadwrite.h or the
bytestream2 API, so not really much in sync beyond all of them being
able to write values of 8 or 16 bits each.

The bytestream2 API will let you clean some code as it handles the
pointer itself as it writes data, but as long as the size calculation
before the av_malloc() call is correct then the bytestream2 overwrite
protection feature wouldn't be useful.
You could use the unchecked bytestream2 API directly, or just keep the
current approach.