[FFmpeg-devel] [RFC] avcodec/avcodec.h: Add encryption info side data

[Jacob Trimble]

On Wed, Dec 20, 2017 at 12:23 PM, wm4 <nfxjfg at googlemail.com> wrote:
> On Wed, 20 Dec 2017 12:07:09 -0800
> Jacob Trimble <modmaker-at-google.com at ffmpeg.org> wrote:
>
>> On Tue, Dec 19, 2017 at 3:05 PM, wm4 <nfxjfg at googlemail.com> wrote:
>> > On Tue, 19 Dec 2017 14:20:38 -0800
>> > Jacob Trimble <modmaker-at-google.com at ffmpeg.org> wrote:
>> >
>> >> > I don't think this is sane. So far, side data could simply be copied
>> >> > with memcpy, and having pointers to non-static data in side data would
>> >> > break this completely
>> >>
>> >> Then how about storing the data like it is now (the encryption info
>> >> followed by the subsample array), but not have a pointer?  Then there
>> >> will be several helper methods to get the subsample array from the
>> >> side-data.  For example,
>> >> av_encryption_info_get_subsamples(AVPacketEncryptionInfo*) or
>> >> av_encryption_info_get_subsamples(AVPacket*) (since there will only be
>> >> one instance of it).
>> >
>> > I guess that would work? Not particularly fond of the idea anyway. I
>> > think the functions would probably work on the side data byte array,
>> > maybe.
>>
>> I'm not fond of this either, but I can't think of a way to allow a
>> dynamic number of elements while supporting memcpy and not requiring
>> the app to "parse" the side-data.
>>
>> So here is may latest attempt.  This has a binary format inside the
>> side-data that allows memcpy to work, but there is a public struct
>> that apps will interact with.  There are two methods used to convert
>> between the two so the app doesn't have to.  Even though this is a
>> binary format, it is not actually a wire format since the subsamples
>> are stored as-is, so they are host byte ordered.  Also, as Michael
>> requested, this uses dynamic sized key ID and IV.
>
> I think your new patch idea is pretty sane. I'd explicitly document
> that the encryption side data format is not part of the ABI, and that
> applications must access it with the av_encryption_info_*() functions.
>
> Also I think it would be better to avoid the memory allocation
> messiness by copying all data, instead of implicitly referencing the
> AVPacket data. Also provide a free function. (That is assuming you're
> fine with a copy - creating AVPacket references copies all side data
> anyway, so the whole code base assumes side data copies are cheap.)
>
> If AVEncryptionInfo ever needs to be extended, it would also be better
> to just let av_encryption_info_get() return a newly allocated struct,
> and to exclude the size of the struct from the ABI. Then adding new
> fields would not require waiting for an ABI bump.

Done
-------------- next part --------------
A non-text attachment was scrubbed...
Name: encryption-info-v5.patch
Type: text/x-patch
Size: 12185 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171220/9ce0bbf9/attachment.bin>