diff mbox

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

Message ID CAO7y9i-iDatywAcL+_ugQdbd9SDFzJui5T8eg9xNs_tb8XVWxw@mail.gmail.com
State Superseded
Headers show

Commit Message

Jacob Trimble Dec. 8, 2017, 6:06 p.m. UTC
On Tue, Dec 5, 2017 at 5:22 PM, Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
> On 12/6/2017 12:36 AM, Jacob Trimble wrote:
>> Would a 0-length array work?  Otherwise I would need to have it be a
>> 1-length array and have to account for that when calculating the size
>> to allocate; it would also require a comment to ignore the size of the
>> array.
>
> Aren't 0-length arrays a GNU extensions? If so, I would gather that it
> probably is not OK in a public header, either.

Yeah.

> I'm not entirely sure what way we prefer nowadays, but you can see
> we've had side data with variable length members before with e.g.
> AV_PKT_DATA_QUALITY_STATS, but that doesn't have a struct associated
> with it. I'm hoping someone more up to date with the side data stuff
> can chime in with a suggestion on what our current best practices
> are for it.

I would prefer to avoid requiring the app to "parse" the side data, using a
plain struct would be better.  I've updated the patch to include my other plan
of allocating a larger bock and having the struct followed by the subsample
array.

I have also renamed the file, I think the mailing list rejected my attachment
before since gmail sent the MIME type as text/x-patch.  Hopefully with the
.txt extension gmail will send the correct MIME type.
From 740c0ccf3ed90b3d417ea25ec26cfefb93974461 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.  To allow for a
variable number of subsamples, the buffer for the side-data will be
allocated to hold both the structure and the array of subsamples.  So
the |subsamples| member will point to right after the struct.

Signed-off-by: Jacob Trimble <modmaker@google.com>
---
 libavcodec/avcodec.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Jacob Trimble Dec. 14, 2017, 5:29 p.m. UTC | #1
On Fri, Dec 8, 2017 at 10:06 AM, Jacob Trimble <modmaker@google.com> wrote:
> On Tue, Dec 5, 2017 at 5:22 PM, Derek Buitenhuis
> <derek.buitenhuis@gmail.com> wrote:
>> On 12/6/2017 12:36 AM, Jacob Trimble wrote:
>>> Would a 0-length array work?  Otherwise I would need to have it be a
>>> 1-length array and have to account for that when calculating the size
>>> to allocate; it would also require a comment to ignore the size of the
>>> array.
>>
>> Aren't 0-length arrays a GNU extensions? If so, I would gather that it
>> probably is not OK in a public header, either.
>
> Yeah.
>
>> I'm not entirely sure what way we prefer nowadays, but you can see
>> we've had side data with variable length members before with e.g.
>> AV_PKT_DATA_QUALITY_STATS, but that doesn't have a struct associated
>> with it. I'm hoping someone more up to date with the side data stuff
>> can chime in with a suggestion on what our current best practices
>> are for it.
>
> I would prefer to avoid requiring the app to "parse" the side data, using a
> plain struct would be better.  I've updated the patch to include my other plan
> of allocating a larger bock and having the struct followed by the subsample
> array.
>
> I have also renamed the file, I think the mailing list rejected my attachment
> before since gmail sent the MIME type as text/x-patch.  Hopefully with the
> .txt extension gmail will send the correct MIME type.

Ping.  Is this something that can be pushed?  I have the implementation for
mov almost ready.  I think having this info exposed would be something that
would be really useful for the project.
Michael Niedermayer Dec. 14, 2017, 10:53 p.m. UTC | #2
On Fri, Dec 08, 2017 at 10:06:13AM -0800, Jacob Trimble wrote:
> On Tue, Dec 5, 2017 at 5:22 PM, Derek Buitenhuis
> <derek.buitenhuis@gmail.com> wrote:
> > On 12/6/2017 12:36 AM, Jacob Trimble wrote:
> >> Would a 0-length array work?  Otherwise I would need to have it be a
> >> 1-length array and have to account for that when calculating the size
> >> to allocate; it would also require a comment to ignore the size of the
> >> array.
> >
> > Aren't 0-length arrays a GNU extensions? If so, I would gather that it
> > probably is not OK in a public header, either.
> 
> Yeah.
> 
> > I'm not entirely sure what way we prefer nowadays, but you can see
> > we've had side data with variable length members before with e.g.
> > AV_PKT_DATA_QUALITY_STATS, but that doesn't have a struct associated
> > with it. I'm hoping someone more up to date with the side data stuff
> > can chime in with a suggestion on what our current best practices
> > are for it.
> 
> I would prefer to avoid requiring the app to "parse" the side data, using a
> plain struct would be better.  I've updated the patch to include my other plan
> of allocating a larger bock and having the struct followed by the subsample
> array.
> 
> I have also renamed the file, I think the mailing list rejected my attachment
> before since gmail sent the MIME type as text/x-patch.  Hopefully with the
> .txt extension gmail will send the correct MIME type.

> From 740c0ccf3ed90b3d417ea25ec26cfefb93974461 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.  To allow for a
> variable number of subsamples, the buffer for the side-data will be
> allocated to hold both the structure and the array of subsamples.  So
> the |subsamples| member will point to right after the struct.
> 
> Signed-off-by: Jacob Trimble <modmaker@google.com>
> ---
>  libavcodec/avcodec.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 5db6a81320..6f5c387556 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1112,6 +1112,53 @@ typedef struct AVCPBProperties {
>      uint64_t vbv_delay;
>  } AVCPBProperties;
>  
> +/**
> + * This describes encryption info for a packet.  This contains frame-specific
> + * info for how to decrypt the packet before passing it to the decoder.  If this
> + * side-data is present, then the packet is encrypted.
> + */

> +typedef struct AVPacketEncryptionInfo {
> +    /** The fourcc encryption scheme. */
> +    int scheme;

uint32_t or unsigned


> +
> +    /** The ID of the key used to encrypt the packet. */
> +    uint8_t key_id[16];
> +
> +    /** The initialization vector. */
> +    uint8_t iv[16];

what happens if the key or iv is longer ?


> +
> +    /**
> +     * Only used for pattern encryption.  This is the number of 16-byte blocks
> +     * that are encrypted.
> +     */
> +    unsigned int crypt_byte_block;
> +
> +    /**
> +     * Only used for pattern encryption.  This is the number of 16-byte blocks
> +     * that are clear.
> +     */
> +    unsigned int skip_byte_block;

why is "16-byte blocks" hardcoded ?


> +
> +    /**
> +     * The number of sub-samples in this packet.  If 0, then the whole sample
> +     * is encrypted.
> +     */
> +    unsigned int subsample_count;
> +
> +    struct {
> +      /** 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;
> +    } *subsamples;

Are these patterns random in practice ?
if not they possibly would be better not repeated per packet


> +} AVPacketEncryptionInfo;

> +#define FF_PACKET_ENCRYPTION_INFO_SIZE(a) (sizeof(AVPacketEncryptionInfo) + sizeof(unsigned int) * a * 2)

This assumes things about the padding and alignment of fields that are not
guranteed by C i think


> +
>  /**
>   * The decoder will keep a reference to the frame and may reuse it later.
>   */
> @@ -1327,6 +1374,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 is an AVPacketEncryptionInfo structure and contains info
> +     * about how the packet is encrypted.
> +     */
> +    AV_PKT_DATA_PACKET_ENCRYPTION_INFO,
> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> -- 
> 2.15.1.424.g9478a66081-goog
> 

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 5db6a81320..6f5c387556 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1112,6 +1112,53 @@  typedef struct AVCPBProperties {
     uint64_t vbv_delay;
 } AVCPBProperties;
 
+/**
+ * This describes encryption info for a packet.  This contains frame-specific
+ * info for how to decrypt the packet before passing it to the decoder.  If this
+ * side-data is present, then the packet is encrypted.
+ */
+typedef struct AVPacketEncryptionInfo {
+    /** The fourcc encryption scheme. */
+    int scheme;
+
+    /** The ID of the key used to encrypt the packet. */
+    uint8_t key_id[16];
+
+    /** The initialization vector. */
+    uint8_t iv[16];
+
+    /**
+     * Only used for pattern encryption.  This is the number of 16-byte blocks
+     * that are encrypted.
+     */
+    unsigned int crypt_byte_block;
+
+    /**
+     * Only used for pattern encryption.  This is the number of 16-byte blocks
+     * that are clear.
+     */
+    unsigned int skip_byte_block;
+
+    /**
+     * The number of sub-samples in this packet.  If 0, then the whole sample
+     * is encrypted.
+     */
+    unsigned int subsample_count;
+
+    struct {
+      /** 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;
+    } *subsamples;
+} AVPacketEncryptionInfo;
+#define FF_PACKET_ENCRYPTION_INFO_SIZE(a) (sizeof(AVPacketEncryptionInfo) + sizeof(unsigned int) * a * 2)
+
 /**
  * The decoder will keep a reference to the frame and may reuse it later.
  */
@@ -1327,6 +1374,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 is an AVPacketEncryptionInfo structure and contains info
+     * about how the packet is encrypted.
+     */
+    AV_PKT_DATA_PACKET_ENCRYPTION_INFO,
+
     /**
      * The number of side data types.
      * This is not part of the public API/ABI in the sense that it may