[FFmpeg-devel,1/3] avformat/mov: Increase support for common encryption.

Submitted by Jacob Trimble on Jan. 30, 2018, 7:27 p.m.

Details

Message ID CAO7y9i_4Xfi3psvt3JacnRHREBY73pK6DJKqE4grF8-L1wkh+Q@mail.gmail.com
State New
Headers show

Commit Message

Jacob Trimble Jan. 30, 2018, 7:27 p.m.
On Wed, Jan 24, 2018 at 5:46 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Wed, Jan 24, 2018 at 11:43:26AM -0800, Jacob Trimble wrote:
>> On Mon, Jan 22, 2018 at 7:38 PM, Michael Niedermayer
>> <michael@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.
>
> I didnt investigate this deeply so there is likely a better option that
> i miss but you could just remove the functions which become unused in a
> subsequent patch to prevent diff from messing the line matching up totally
>

Done.

>
>>
>> >
>> >>
>> >> Signed-off-by: Jacob Trimble <modmaker@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?
>
> If you post a new version, then there should be a mail or comment explaining
> any dependancies on yet to be applied patches.
> It should not be in the commit messages or commited changes ideally
> This way people trying to test code dont need to guess what they need
> to apply first before a patchset
>
>
> [...]
>> >> +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.
>
> Indention depth is 4 in mov*.c
> the hunk seems to add lines with a depth of 2
> I would be surprised if this is not in the file after applying the patch
>
> personally i dont care about the depth that much but i know many other people
> care so this needs to be fixed before this can be applied

Didn't see that.  Fixed and did a grep for incorrect indentations.

>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Comments

Jacob Trimble Feb. 12, 2018, 5:35 p.m.
On Tue, Jan 30, 2018 at 11:27 AM, Jacob Trimble <modmaker@google.com> wrote:
> On Wed, Jan 24, 2018 at 5:46 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>> On Wed, Jan 24, 2018 at 11:43:26AM -0800, Jacob Trimble wrote:
>>> On Mon, Jan 22, 2018 at 7:38 PM, Michael Niedermayer
>>> <michael@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.
>>
>> I didnt investigate this deeply so there is likely a better option that
>> i miss but you could just remove the functions which become unused in a
>> subsequent patch to prevent diff from messing the line matching up totally
>>
>
> Done.
>
>>
>>>
>>> >
>>> >>
>>> >> Signed-off-by: Jacob Trimble <modmaker@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?
>>
>> If you post a new version, then there should be a mail or comment explaining
>> any dependancies on yet to be applied patches.
>> It should not be in the commit messages or commited changes ideally
>> This way people trying to test code dont need to guess what they need
>> to apply first before a patchset
>>
>>
>> [...]
>>> >> +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.
>>
>> Indention depth is 4 in mov*.c
>> the hunk seems to add lines with a depth of 2
>> I would be surprised if this is not in the file after applying the patch
>>
>> personally i dont care about the depth that much but i know many other people
>> care so this needs to be fixed before this can be applied
>
> Didn't see that.  Fixed and did a grep for incorrect indentations.
>
>>
>> [...]
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Let us carefully observe those good qualities wherein our enemies excel us
>> and endeavor to excel them, by avoiding what is faulty, and imitating what
>> is excellent in them. -- Plutarch
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>

Ping.  This depends on
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-January/223754.html.

Patch hide | download patch | download mbox

From 8de26af99003646f3abfb8ee74d5d0bec7d20d08 Mon Sep 17 00:00:00 2001
From: Jacob Trimble <modmaker@google.com>
Date: Tue, 30 Jan 2018 11:14:35 -0800
Subject: [PATCH] avformat/mov: Remove old encryption info methods.

Signed-off-by: Jacob Trimble <modmaker@google.com>
---
 libavformat/isom.h | 10 ------
 libavformat/mov.c  | 91 ------------------------------------------------------
 2 files changed, 101 deletions(-)

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 42b9426c11..3794b1f0fd 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -223,16 +223,6 @@  typedef struct MOVStreamContext {
 
     int has_sidx;  // If there is an sidx entry for this stream.
     struct {
-        // TODO: Remove once old methods are removed from mov.c
-        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;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3b791b8599..f0f52b765a 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5957,67 +5957,6 @@  static int mov_read_tenc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     return 0;
 }
 
-static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
-{
-    AVStream *st;
-    MOVStreamContext *sc;
-    size_t data_size;
-    int atom_header_size;
-    int flags;
-
-    if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
-        return 0;
-
-    st = c->fc->streams[c->fc->nb_streams - 1];
-    sc = st->priv_data;
-
-    if (sc->cenc.auxiliary_info_sizes || sc->cenc.auxiliary_info_default_size) {
-        av_log(c->fc, AV_LOG_ERROR, "duplicate saiz atom\n");
-        return AVERROR_INVALIDDATA;
-    }
-
-    atom_header_size = 9;
-
-    avio_r8(pb); /* version */
-    flags = avio_rb24(pb);
-
-    if ((flags & 0x01) != 0) {
-        atom_header_size += 8;
-
-        avio_rb32(pb);    /* info type */
-        avio_rb32(pb);    /* info type param */
-    }
-
-    sc->cenc.auxiliary_info_default_size = avio_r8(pb);
-    avio_rb32(pb);    /* entries */
-
-    if (atom.size <= atom_header_size) {
-        return 0;
-    }
-
-    if (atom.size > FFMIN(INT_MAX, SIZE_MAX)) {
-        av_log(c->fc, AV_LOG_ERROR, "saiz atom auxiliary_info_sizes size %"PRId64" invalid\n", atom.size);
-        return AVERROR_INVALIDDATA;
-    }
-
-    /* save the auxiliary info sizes as is */
-    data_size = atom.size - atom_header_size;
-
-    sc->cenc.auxiliary_info_sizes = av_malloc(data_size);
-    if (!sc->cenc.auxiliary_info_sizes) {
-        return AVERROR(ENOMEM);
-    }
-
-    sc->cenc.auxiliary_info_sizes_count = data_size;
-
-    if (avio_read(pb, sc->cenc.auxiliary_info_sizes, data_size) != data_size) {
-        av_log(c->fc, AV_LOG_ERROR, "failed to read the auxiliary info sizes");
-        return AVERROR_INVALIDDATA;
-    }
-
-    return 0;
-}
-
 static int mov_read_dfla(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     AVStream *st;
@@ -6055,36 +5994,6 @@  static int mov_read_dfla(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     return 0;
 }
 
-static int mov_seek_auxiliary_info(MOVContext *c, MOVStreamContext *sc, int64_t index)
-{
-    size_t auxiliary_info_seek_offset = 0;
-    int i;
-
-    if (sc->cenc.auxiliary_info_default_size) {
-        auxiliary_info_seek_offset = (size_t)sc->cenc.auxiliary_info_default_size * index;
-    } else if (sc->cenc.auxiliary_info_sizes) {
-        if (index > sc->cenc.auxiliary_info_sizes_count) {
-            av_log(c, AV_LOG_ERROR, "current sample %"PRId64" greater than the number of auxiliary info sample sizes %"SIZE_SPECIFIER"\n",
-                index, sc->cenc.auxiliary_info_sizes_count);
-            return AVERROR_INVALIDDATA;
-        }
-
-        for (i = 0; i < index; i++) {
-            auxiliary_info_seek_offset += sc->cenc.auxiliary_info_sizes[i];
-        }
-    }
-
-    if (auxiliary_info_seek_offset > sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info) {
-        av_log(c, AV_LOG_ERROR, "auxiliary info offset %"SIZE_SPECIFIER" greater than auxiliary info size %"SIZE_SPECIFIER"\n",
-            auxiliary_info_seek_offset, (size_t)(sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info));
-        return AVERROR_INVALIDDATA;
-    }
-
-    sc->cenc.auxiliary_info_pos = sc->cenc.auxiliary_info + auxiliary_info_seek_offset;
-    sc->cenc.auxiliary_info_index = index;
-    return 0;
-}
-
 static int cenc_decrypt(MOVContext *c, MOVStreamContext *sc, AVEncryptionInfo *sample, uint8_t *input, int size)
 {
     int i, ret;
-- 
2.16.0.rc1.238.g530d649a79-goog