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

[Jacob Trimble]

On Mon, Mar 5, 2018 at 12:22 PM, Jacob Trimble <modmaker at google.com> wrote:
> On Mon, Feb 12, 2018 at 9:35 AM, Jacob Trimble <modmaker at google.com> wrote:
>> On Tue, Jan 30, 2018 at 11:27 AM, Jacob Trimble <modmaker at google.com> wrote:
>>> On Wed, Jan 24, 2018 at 5:46 PM, Michael Niedermayer
>>> <michael at 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 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.
>>>>
>>>> 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 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?
>>>>
>>>> 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 at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>
>> Ping.  This depends on
>> http://ffmpeg.org/pipermail/ffmpeg-devel/2018-January/223754.html.
>
> Ping again.  I know this is low priority, but I would like to get
> these merged soon.

Ping.  Despite being almost 2 months old, these patches still apply
cleanly.  Please take a look.  These have been in review for almost 3
months.