On Fri, Jan 05, 2018 at 10:22:31AM -0800, Jacob Trimble wrote:
> On Tue, Jan 2, 2018 at 9:57 AM, Jacob Trimble <modma...@google.com> wrote:
> > On Wed, Dec 20, 2017 at 4:31 PM, wm4 <nfx...@googlemail.com> wrote:
> >> On Wed, 20 Dec 2017 15:10:43 -0800
> >> Jacob Trimble <modmaker-at-google....@ffmpeg.org> wrote:
> >>
> >>> From 1508d19e9f7acf43d76010ce54d59ff204613601 Mon Sep 17 00:00:00 2001
> >>> From: Jacob Trimble <modma...@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 <modma...@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,
> >
> > Leaving version bump and changelog entry to whoever pushes it, but
> > other comments done.
>
> Just noticed the new files were in libavcodec, moved to libavutil.
> Can someone please review this so it can be pushed. I have the MP4
> implementation ready and I would like to get that reviewed and pushed
> as soon as possible.
>
> Thanks.
> libavcodec/avcodec.h | 13 ++++
> libavutil/Makefile | 2
> libavutil/encryption_info.c | 137
> ++++++++++++++++++++++++++++++++++++++++++++
> libavutil/encryption_info.h | 121 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 273 insertions(+)
> 44c4062d454970ff3ff037a101beaeab99a4e60b encryption-info-v7.patch
> From 758b2439e779045fbc82ca8cf91c7f71230e440b Mon Sep 17 00:00:00 2001
> From: Jacob Trimble <modma...@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 <modma...@google.com>
> ---
> libavcodec/avcodec.h | 13 +++++
> libavutil/Makefile | 2 +
> libavutil/encryption_info.c | 137
> ++++++++++++++++++++++++++++++++++++++++++++
> libavutil/encryption_info.h | 121 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 273 insertions(+)
> create mode 100644 libavutil/encryption_info.c
> create mode 100644 libavutil/encryption_info.h
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index c13deb599f..e766f21ca0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1341,6 +1341,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/libavutil/Makefile b/libavutil/Makefile
> index 66b894d66e..fed936df7b 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -24,6 +24,7 @@ HEADERS = adler32.h
> \
> dict.h \
> display.h \
> downmix_info.h \
> + encryption_info.h \
> error.h \
> eval.h \
> fifo.h \
> @@ -107,6 +108,7 @@ OBJS = adler32.o
> \
> dict.o \
> display.o \
> downmix_info.o \
> + encryption_info.o \
> error.o \
> eval.o \
> fifo.o \
> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
> new file mode 100644
> index 0000000000..76796b50ee
> --- /dev/null
> +++ b/libavutil/encryption_info.c
> @@ -0,0 +1,137 @@
> +/**
> + * 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/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)
> +{
> + 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_encryption_info_free(info);
> + return NULL;
> + }
> +
> + 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_get_side_data(const AVPacket *packet)
> +{
> + 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);
These should be checked against size, making sure the data is consistent
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
