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

On Wed, Dec 20, 2017 at 12:23 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Wed, 20 Dec 2017 12:07:09 -0800
> Jacob Trimble <modmaker-at-google.com@ffmpeg.org> wrote:
>
>> On Tue, Dec 19, 2017 at 3:05 PM, wm4 <nfxjfg@googlemail.com> wrote:
>> > On Tue, 19 Dec 2017 14:20:38 -0800
>> > Jacob Trimble <modmaker-at-google.com@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

On Wed, 20 Dec 2017 15:10:43 -0800
Jacob Trimble <modmaker-at-google.com@ffmpeg.org> wrote:

> From 1508d19e9f7acf43d76010ce54d59ff204613601 Mon Sep 17 00:00:00 2001
> From: Jacob Trimble <modmaker@google.com>
> Date: Tue, 5 Dec 2017 14:52:22 -0800
> Subject: [PATCH] avcodec/avcodec.h: Add encryption info side data.
> 
> This new side-data will contain info on how a packet is encrypted.
> This allows the app to handle packet decryption.
> 
> Signed-off-by: Jacob Trimble <modmaker@google.com>
> ---

Looks generally good to me, a few minor cosmetic comments below.

>  libavcodec/Makefile          |   2 +
>  libavcodec/avcodec.h         |  13 ++++
>  libavcodec/encryption_info.c | 139 +++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/encryption_info.h | 121 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 275 insertions(+)
>  create mode 100644 libavcodec/encryption_info.c
>  create mode 100644 libavcodec/encryption_info.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index ab7893f560..11ad642c6c 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -10,6 +10,7 @@ HEADERS = ac3_parser.h                                                  \
>            dirac.h                                                       \
>            dv_profile.h                                                  \
>            dxva2.h                                                       \
> +          encryption_info.h                                             \
>            jni.h                                                         \
>            mediacodec.h                                                  \
>            qsv.h                                                         \
> @@ -36,6 +37,7 @@ OBJS = ac3_parser.o                                                     \
>         dirac.o                                                          \
>         dv_profile.o                                                     \
>         encode.o                                                         \
> +       encryption_info.o                                                \
>         imgconvert.o                                                     \
>         jni.o                                                            \
>         mathtables.o                                                     \
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 5db6a81320..b43638ebc5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1327,6 +1327,19 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_A53_CC,
>  
> +    /**
> +     * This side data is encryption "initialization data".
> +     * For MP4 this is the entire 'pssh' box.
> +     * For WebM this is the key ID.
> +     */
> +    AV_PKT_DATA_ENCRYPTION_INIT_DATA,
> +
> +    /**
> +     * This side data contains encryption info for how to decrypt the packet.
> +     * The format is not part of ABI, use av_encryption_info_* methods to access.
> +     */
> +    AV_PKT_DATA_ENCRYPTION_INFO,
> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> diff --git a/libavcodec/encryption_info.c b/libavcodec/encryption_info.c
> new file mode 100644
> index 0000000000..59ee4c41a9
> --- /dev/null
> +++ b/libavcodec/encryption_info.c
> @@ -0,0 +1,139 @@
> +/**
> + * 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 "encryption_info.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/intreadwrite.h"
> +
> +#define FF_ENCRYPTION_INFO_EXTRA 24
> +
> +// The format of the side data:
> +// u32be scheme
> +// u32be crypt_byte_block
> +// u32be skip_byte_block
> +// u32be key_id_size
> +// u32be iv_size
> +// u32be subsample_count
> +// u8[key_id_size] key_id
> +// u8[iv_size] iv
> +// {
> +//   u32be bytes_of_clear_data
> +//   u32be bytes_of_protected_data
> +// }[subsample_count]
> +
> +AVEncryptionInfo* av_encryption_info_alloc(uint32_t subsample_count, uint32_t key_id_size, uint32_t iv_size)

I think we generally put the "*" to the name, to reflect the actual C
syntax. (Here and in other places where pointers are in the function
return or argument types.)

> +{
> +    AVEncryptionInfo *info;
> +
> +    info = av_mallocz(sizeof(AVEncryptionInfo));
> +    if (!info)
> +        return NULL;
> +
> +    info->key_id = av_mallocz(key_id_size);
> +    info->key_id_size = key_id_size;
> +    info->iv = av_mallocz(iv_size);
> +    info->iv_size = iv_size;
> +    info->subsamples = av_mallocz_array(sizeof(AVSubsampleEncryptionInfo), subsample_count);
> +    info->subsample_count = subsample_count;
> +
> +    // Allow info->subsamples to be NULL if there are no subsamples.
> +    if (!info->key_id || !info->iv || (!info->subsamples && subsample_count)) {
> +        av_free(info->key_id);
> +        av_free(info->iv);
> +        av_free(info->subsamples);

Couldn't this just call av_encryption_info_free()?

> +        av_freep(&info);
> +    }
> +
> +    return info;
> +}
> +
> +void av_encryption_info_free(AVEncryptionInfo* info)
> +{
> +    if (info) {
> +        av_free(info->key_id);
> +        av_free(info->iv);
> +        av_free(info->subsamples);
> +        av_free(info);
> +    }
> +}
> +
> +AVEncryptionInfo* av_encryption_info_copy_side_data(const AVPacket* packet)

Could still be called "get" instead of "copy". I get the intention that
this returns a copy of the data, but "copy" still sounds like it's
being copied somewhere else. But I have no strong feelings about it.

> +{
> +    AVEncryptionInfo *info;
> +    uint8_t *buffer;
> +    unsigned int size;
> +    uint32_t key_id_size, iv_size, subsample_count, i;
> +
> +    buffer = av_packet_get_side_data(packet, AV_PKT_DATA_ENCRYPTION_INFO, &size);
> +    if (!buffer)
> +        return NULL;
> +
> +    key_id_size = AV_RB32(buffer + 12);
> +    iv_size = AV_RB32(buffer + 16);
> +    subsample_count = AV_RB32(buffer + 20);
> +
> +    info = av_encryption_info_alloc(subsample_count, key_id_size, iv_size);
> +    if (!info)
> +        return NULL;
> +
> +    info->scheme = AV_RB32(buffer);
> +    info->crypt_byte_block = AV_RB32(buffer + 4);
> +    info->skip_byte_block = AV_RB32(buffer + 8);
> +    memcpy(info->key_id, buffer + 24, key_id_size);
> +    memcpy(info->iv, buffer + key_id_size + 24, iv_size);
> +
> +    buffer += key_id_size + iv_size + 24;
> +    for (i = 0; i < subsample_count; i++) {
> +        info->subsamples[i].bytes_of_clear_data = AV_RB32(buffer);
> +        info->subsamples[i].bytes_of_protected_data = AV_RB32(buffer + 4);
> +        buffer += 8;
> +    }
> +
> +    return info;
> +}
> +
> +int av_encryption_info_add_side_data(AVPacket* packet, const AVEncryptionInfo* info) {

We put the "{" on a separate line for functions.

> +    uint8_t *buffer;
> +    size_t size;
> +    uint32_t i;
> +
> +    size = FF_ENCRYPTION_INFO_EXTRA + info->key_id_size + info->iv_size +
> +           (sizeof(AVSubsampleEncryptionInfo) * info->subsample_count);
> +    buffer = av_packet_new_side_data(packet, AV_PKT_DATA_ENCRYPTION_INFO, size);
> +    if (!buffer)
> +        return AVERROR(ENOMEM);
> +
> +    AV_WB32(buffer,      info->scheme);
> +    AV_WB32(buffer +  4, info->crypt_byte_block);
> +    AV_WB32(buffer +  8, info->skip_byte_block);
> +    AV_WB32(buffer + 12, info->key_id_size);
> +    AV_WB32(buffer + 16, info->iv_size);
> +    AV_WB32(buffer + 20, info->subsample_count);
> +    buffer += 24;
> +    memcpy(buffer, info->key_id, info->key_id_size);
> +    buffer += info->key_id_size;
> +    memcpy(buffer, info->iv, info->iv_size);
> +    buffer += info->iv_size;
> +    for (i = 0; i < info->subsample_count; i++) {
> +        AV_WB32(buffer, info->subsamples[i].bytes_of_clear_data);
> +        AV_WB32(buffer + 4, info->subsamples[i].bytes_of_protected_data);
> +        buffer += 8;
> +    }
> +
> +    return 0;
> +}
> diff --git a/libavcodec/encryption_info.h b/libavcodec/encryption_info.h
> new file mode 100644
> index 0000000000..4fc9b6d3f1
> --- /dev/null
> +++ b/libavcodec/encryption_info.h
> @@ -0,0 +1,121 @@
> +/**
> + * 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
> + */
> +
> +#ifndef AVUTIL_ENCRYPTION_INFO_H
> +#define AVUTIL_ENCRYPTION_INFO_H
> +
> +#include "avcodec.h"
> +
> +typedef struct AVSubsampleEncryptionInfo {
> +    /** The number of bytes that are clear. */
> +    unsigned int bytes_of_clear_data;
> +
> +    /**
> +     * The number of bytes that are protected.  If using pattern encryption,
> +     * the pattern applies to only the protected bytes; if not using pattern
> +     * encryption, all these bytes are encrypted.
> +     */
> +    unsigned int bytes_of_protected_data;
> +} AVSubsampleEncryptionInfo;
> +
> +/**
> + * This describes encryption info for a packet.  This contains frame-specific
> + * info for how to decrypt the packet before passing it to the decoder.
> + *
> + * The size of this struct is not part of the public ABI.
> + */
> +typedef struct AVEncryptionInfo {
> +    /** The fourcc encryption scheme. */
> +    uint32_t scheme;
> +
> +    /**
> +     * Only used for pattern encryption.  This is the number of 16-byte blocks
> +     * that are encrypted.
> +     */
> +    uint32_t crypt_byte_block;
> +
> +    /**
> +     * Only used for pattern encryption.  This is the number of 16-byte blocks
> +     * that are clear.
> +     */
> +    uint32_t skip_byte_block;
> +
> +    /**
> +     * The ID of the key used to encrypt the packet.  This should always be
> +     * 16 bytes long, but may be changed in the future.
> +     */
> +    uint8_t *key_id;
> +    uint32_t key_id_size;
> +
> +    /**
> +     * The initialization vector.  This may have been zero-filled to be the
> +     * correct block size.  This should always be 16 bytes long, but may be
> +     * changed in the future.
> +     */
> +    uint8_t *iv;
> +    uint32_t iv_size;
> +
> +    /**
> +     * An array of subsample encryption info specifying how parts of the sample
> +     * are encrypted.  If there are no subsamples, then the whole sample is
> +     * encrypted.
> +     */
> +    AVSubsampleEncryptionInfo* subsamples;
> +    uint32_t subsample_count;
> +} AVEncryptionInfo;
> +
> +/**
> + * Allocates an AVEncryptionInfo structure and sub-pointers to hold the given
> + * number of subsamples.  This will allocate pointers for the key ID, IV,
> + * and subsample entries, set the size members, and zero-initialize the rest.
> + *
> + * @param subsample_count The number of subsamples.
> + * @param key_id_size The number of bytes in the key ID, should be 16.
> + * @param key_id_size The number of bytes in the IV, should be 16.
> + *
> + * @return The new AVEncryptionInfo structure, or NULL on error.
> + */
> +AVEncryptionInfo* av_encryption_info_alloc(uint32_t subsample_count, uint32_t key_id_size, uint32_t iv_size);
> +
> +/**
> + * Frees the given encryption info object.  This MUST NOT be used to free the
> + * side-data data pointer, that should use normal side-data methods.
> + */
> +void av_encryption_info_free(AVEncryptionInfo* info);
> +
> +/**
> + * Creates a copy of the AVEncryptionInfo that is contained in the side data of
> + * the given packet.  The resulting object should be passed to
> + * av_encryption_info_free() when done.
> + *
> + * This returns NULL if there is a memory error or if the packet isn't encrypted.
> + * To detect if the packet is encrypted, use av_packet_get_side_data.
> + *
> + * @return The new AVEncryptionInfo structure, or NULL on error.
> + */
> +AVEncryptionInfo* av_encryption_info_copy_side_data(const AVPacket* packet);
> +
> +/**
> + * Adds a new side data to the given packet that holds a copy of the given
> + * encryption info.
> + *
> + * @return 0 on success, or a negative error code on error.
> + */
> +int av_encryption_info_add_side_data(AVPacket* packet, const AVEncryptionInfo* info);
> +
> +#endif /* AVUTIL_ENCRYPTION_INFO_H */

So as far as the side data management goes, this should be good. No
comment about the crypto stuff or the byte array (de)serialization
stuff, didn't take a close look at it. The final patch should also
include a minor libavutil bump and an doc/APIchanges entry,