[FFmpeg-devel] [PATCH 1/3] avformat/mov: Increase support for common encryption.
Jacob Trimble
modmaker at google.com
Wed Jan 24 21:43:26 EET 2018
On Mon, Jan 22, 2018 at 7:38 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote
> [...]
>> This removes support for saio/saiz atoms, but it was incorrect before.
>> A follow-up change will add correct support for those.
>
> This removal should be done by a seperate patch if it is done.
> diff has matched up the removed function with a added one making this
> hard to read as is
>
The problem is that the old code used the saiz atoms to parse the senc
atom. I split the patch up for readability, but the two patches need
to be applied at the same time (or squashed) since the first breaks
encrypted content. But I can squash them again if it is preferable to
not have a commit that intentionally breaks things.
>
>>
>> Signed-off-by: Jacob Trimble <modmaker at google.com>
>> ---
>> libavformat/isom.h | 20 +-
>> libavformat/mov.c | 432 ++++++++++++++++++++++-----------
>> tests/fate/mov.mak | 8 +
>> tests/ref/fate/mov-frag-encrypted | 57 +++++
>> tests/ref/fate/mov-tenc-only-encrypted | 57 +++++
>> 5 files changed, 422 insertions(+), 152 deletions(-)
>> create mode 100644 tests/ref/fate/mov-frag-encrypted
>> create mode 100644 tests/ref/fate/mov-tenc-only-encrypted
>
> This depends on other patches you posted, this should be mentioned or
> all patches should be in the same patchset in order
>
This depends on
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-January/223754.html and
the recently pushed change to libavutil/aes_ctr. Should I add
something to the commit message or is that enough?
>
>>
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index 65676fb0f5..3794b1f0fd 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -27,6 +27,7 @@
>> #include <stddef.h>
>> #include <stdint.h>
>>
>> +#include "libavutil/encryption_info.h"
>> #include "libavutil/mastering_display_metadata.h"
>> #include "libavutil/spherical.h"
>> #include "libavutil/stereo3d.h"
>> @@ -108,12 +109,20 @@ typedef struct MOVSbgp {
>> unsigned int index;
>> } MOVSbgp;
>>
>> +typedef struct MOVEncryptionIndex {
>> + // Individual encrypted samples. If there are no elements, then the default
>> + // settings will be used.
>> + unsigned int nb_encrypted_samples;
>> + AVEncryptionInfo **encrypted_samples;
>> +} MOVEncryptionIndex;
>> +
>> typedef struct MOVFragmentStreamInfo {
>> int id;
>> int64_t sidx_pts;
>> int64_t first_tfra_pts;
>> int64_t tfdt_dts;
>> int index_entry;
>> + MOVEncryptionIndex *encryption_index;
>> } MOVFragmentStreamInfo;
>>
>> typedef struct MOVFragmentIndexItem {
>> @@ -214,15 +223,10 @@ typedef struct MOVStreamContext {
>>
>> int has_sidx; // If there is an sidx entry for this stream.
>> struct {
>> - int use_subsamples;
>> - uint8_t* auxiliary_info;
>> - uint8_t* auxiliary_info_end;
>> - uint8_t* auxiliary_info_pos;
>> - uint8_t auxiliary_info_default_size;
>> - uint8_t* auxiliary_info_sizes;
>> - size_t auxiliary_info_sizes_count;
>> - int64_t auxiliary_info_index;
>> struct AVAESCTR* aes_ctr;
>> + unsigned int per_sample_iv_size; // Either 0, 8, or 16.
>> + AVEncryptionInfo *default_encrypted_sample;
>> + MOVEncryptionIndex *encryption_index;
>> } cenc;
>> } MOVStreamContext;
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 22faecfc17..37320af2f6 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1324,6 +1324,7 @@ static int update_frag_index(MOVContext *c, int64_t offset)
>> frag_stream_info[i].tfdt_dts = AV_NOPTS_VALUE;
>> frag_stream_info[i].first_tfra_pts = AV_NOPTS_VALUE;
>> frag_stream_info[i].index_entry = -1;
>> + frag_stream_info[i].encryption_index = NULL;
>> }
>>
>> if (index < c->frag_index.nb_items)
>> @@ -5710,117 +5711,252 @@ static int mov_read_frma(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> return 0;
>> }
>>
>> -static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +/**
>> + * Gets the current encryption info and associated current stream context. If
>> + * we are parsing a track fragment, this will return the specific encryption
>> + * info for this fragment; otherwise this will return the global encryption
>> + * info for the current stream.
>> + */
>
>> +static int get_current_encryption_info(MOVContext *c, MOVEncryptionIndex **encryption_index, MOVStreamContext **sc)
>> {
>> + MOVFragmentStreamInfo *frag_stream_info;
>> AVStream *st;
>> - MOVStreamContext *sc;
>> - size_t auxiliary_info_size;
>> + int i;
>>
>> - if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
>> - return 0;
>> + frag_stream_info = get_current_frag_stream_info(&c->frag_index);
>> + if (frag_stream_info) {
>> + for (i = 0; i < c->fc->nb_streams; i++) {
>> + if (c->fc->streams[i]->id == frag_stream_info->id) {
>> + st = c->fc->streams[i];
>> + break;
>> + }
>> + }
>
> the indention is inconsistent here
>
No it's not, it looks like it because the diff looks odd. If you
apply the patch, the indentation in this method is consistent.
>
> [...]
>
>> +static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> + AVEncryptionInfo **encrypted_samples;
>> + MOVEncryptionIndex *encryption_index;
>> + MOVStreamContext *sc;
>> + int use_subsamples, ret;
>> + unsigned int sample_count, i, alloc_size = 0;
>>
>> - if (avio_read(pb, sc->cenc.auxiliary_info, auxiliary_info_size) != auxiliary_info_size) {
>> - av_log(c->fc, AV_LOG_ERROR, "failed to read the auxiliary info");
>> - return AVERROR_INVALIDDATA;
>> + ret = get_current_encryption_info(c, &encryption_index, &sc);
>> + if (ret != 1)
>> + return ret;
>> +
>> + if (encryption_index->nb_encrypted_samples) {
>> + // This can happen if we have both saio/saiz and senc atoms.
>> + av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in senc\n");
>> + return 0;
>> }
>>
>> - /* initialize the cipher */
>> - sc->cenc.aes_ctr = av_aes_ctr_alloc();
>> - if (!sc->cenc.aes_ctr) {
>> + avio_r8(pb); /* version */
>> + use_subsamples = avio_rb24(pb) & 0x02; /* flags */
>> +
>> + sample_count = avio_rb32(pb);
>> + if (sample_count >= INT_MAX / sizeof(*encrypted_samples))
>> return AVERROR(ENOMEM);
>> +
>> + for (i = 0; i < sample_count && !pb->eof_reached; i++) {
>> + unsigned int min_samples = FFMIN(FFMAX(i, 1024 * 1024), sample_count);
>> + encrypted_samples = av_fast_realloc(encryption_index->encrypted_samples, &alloc_size,
>> + min_samples * sizeof(*encrypted_samples));
>> + if (!encrypted_samples) {
>> + ret = AVERROR(ENOMEM);
>> + goto end;
>> + }
>> + encryption_index->encrypted_samples = encrypted_samples;
>> +
>> + ret = mov_read_sample_encryption_info(c, pb, sc, &encryption_index->encrypted_samples[i], use_subsamples);
>> + if (ret < 0) {
>> + goto end;
>> + }
>> + }
>> +
>> + if (pb->eof_reached) {
>> + av_log(c->fc, AV_LOG_ERROR, "Hit EOF while reading senc\n");
>> + ret = AVERROR_INVALIDDATA;
>> }
>>
>> - return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
>> +end:
>> + if (ret < 0) {
>> + for (; i > 0; i--)
>> + av_encryption_info_free(encryption_index->encrypted_samples[i - 1]);
>
> I think its a bit risky to use "i" here like this.
> if someone adds a goto end before i is first used this breaks
> if someone adds a loop after the main loop this breaks too
>
Yeah, you're right. Changed to free the partial array in the loop.
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0000-avformat-mov-Remove-broken.patch
Type: text/x-patch
Size: 5347 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180124/bad9b932/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-mov-Increase-support-for-v5.patch
Type: text/x-patch
Size: 31626 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180124/bad9b932/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list