From patchwork Tue Jan 30 19:27:00 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacob Trimble X-Patchwork-Id: 7451 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.156.27 with SMTP id q27csp738813jak; Tue, 30 Jan 2018 11:33:35 -0800 (PST) X-Google-Smtp-Source: AH8x226EJqZsc6U/vB/Yi+7gfOp3N87XqSOXX/1NC/y3g6BXL62ytJ/A5RktmCE0e7WVNMeuXj05 X-Received: by 10.223.183.46 with SMTP id l46mr8892430wre.33.1517340815071; Tue, 30 Jan 2018 11:33:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517340815; cv=none; d=google.com; s=arc-20160816; b=xDRgwsJQvGL/pztkNTIek9fLabxLORBDXb/DF+ZRTyiP6X/KOrTiPUOmduvdj/WRBq +mvDNAlyC2XvPzpLQCBu3rQVIydEbp/CXm5CoMTIs6JPBzQLT/B3lCd7lRRS9m+TZ/Hh sQHuf86FGEssX2A7sneZl9Dym4sObc35zsOMXQ+Emzn570TZePi+OtUgKhnMX5p0T1fp V4waFoqP4Vy29vmUayJTOkFxoDEuvtDqhgDSBqaIRxzwXPVDsliUqvwE6crGu7Hl0pja 6eJiT4QG65wEjWlYNveGTUGIX6qsnk4hIOuIsMVBwkNzqX4quNugFI//h9hFOYFctt43 qvbg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:to :message-id:date:from:references:in-reply-to:mime-version :dkim-signature:delivered-to:arc-authentication-results; bh=yyEq7ldisSCXa/ZAnxEqTNMZ2WzYRCUFarSKPr4DNRQ=; b=vHcefQzZPpXcJ7DsIvR1jugqv2ID5r/rVHqTwp9ONTLDwiT5jn7X1v9lhd1Klc7UTt AZ9vKi6zSmG0qJ6dMhOqDNXs5HIWe2GVSxkoaS8rEBV/NU/IzIUtV4yZVLz7agLz16MV grwXQq0OZ3Fr4nRuZoWvCVTNRVpSJjDg6IG658j22iyqelJTowMw9wg2jt1eRZ7WNjwA +5LMVXZk2Q6KEbkUaNDuuE1mm5HLuNH2A4yniwcX0aNR/QkFaS77z5bUNTw/KlAMs3mJ Q5QwlQku4gt01JY4up3QEC9A9kKYM24mmaStw0oLdd3Jt6eLWkN5cTry0ayjXBCzWT2t J19w== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.s=20161025 header.b=nWrlIwOF; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id q127si9743903wma.6.2018.01.30.11.33.33; Tue, 30 Jan 2018 11:33:34 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.s=20161025 header.b=nWrlIwOF; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A25276883D2; Tue, 30 Jan 2018 21:33:27 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-lf0-f51.google.com (mail-lf0-f51.google.com [209.85.215.51]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 88CFD680757 for ; Tue, 30 Jan 2018 21:33:20 +0200 (EET) Received: by mail-lf0-f51.google.com with SMTP id o89so17055177lfg.10 for ; Tue, 30 Jan 2018 11:33:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=ZXJFKMJl5HEuSQKkEg4GHrt73Td0lkFrGqKPeb4sXoQ=; b=nWrlIwOFMui3L3MPHKoYkm9smpe1BmSU9AX67LLr/clTJ2nwKUFmE3uuKG/9oNJApg OKwjQSAeQ72e8df11miBiWSM4h4bE96RZHdyPP6ELR//XNrct2/6kgyAjFa0E7DkLM1B eRAu1lkh/8qmk7dCey7NwSgtSToCWS+HDjKDKorQ7GF+iFjvPjxmsw74ETjgk2Oa7Cim 10Vb6r2cQFtK47cenMfhEWJ4uhud0tEl1YdrnSGANuijRZHwXdvY7sMlvk9O4i22cbIG yLFlvVz5YY6/HY2Jze9eXDMBLDXKgxvewFxPXSlTi+ZsQkJYttp0EHWMuA+czQ5bTj69 QLow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=ZXJFKMJl5HEuSQKkEg4GHrt73Td0lkFrGqKPeb4sXoQ=; b=ar8UF3VOHAPzJ1CRJHVFS+TObgkdEF0qqT/zE84gtxxW7iDstGRIc6EkyJ5XDN82nh 9TvYnfaWk2xpc0UwtDFvKUzQbMQHbi/rcOCcnTWs3VQJN7DQjUObaH+pwsP66yztHGLD LjId9FP6tM6xtaVmf8j0tDliVu8NuEGQIiTyQO/7n9vI1DjclBumHl30ObU6JLLk4Ao6 sTiDmt1qiDZennb9zNCQPnu69jEXvCLR6rkq3Zs5I2UR/TIaGvV9Ej6yZkVUDvKjb8Dv 2PAC/AANQV6vl7jLBRII3gK0VwLv3jyObv8adwhbK0uwHdMHYXzcp5sfBZ+w3/e8Nh4N JTRQ== X-Gm-Message-State: AKwxytcOFW2n7qYVVl3EkI+ROzXRj6cOz6yo76L0nlztEX+H6oKkTxVh 5eQwYKEKVgnXOrMW1Rz5NI3GjC+nkpg7qhoTlVsFPG6n X-Received: by 10.46.70.10 with SMTP id t10mr16071519lja.27.1517340422085; Tue, 30 Jan 2018 11:27:02 -0800 (PST) MIME-Version: 1.0 Received: by 10.46.4.208 with HTTP; Tue, 30 Jan 2018 11:27:00 -0800 (PST) In-Reply-To: <20180125014605.GT3428@michaelspb> References: <20180105194928.104085-1-modmaker@google.com> <20180110215135.GX4926@michaelspb> <20180123033815.GN3428@michaelspb> <20180125014605.GT3428@michaelspb> From: Jacob Trimble Date: Tue, 30 Jan 2018 11:27:00 -0800 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH 1/3] avformat/mov: Increase support for common encryption. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On Wed, Jan 24, 2018 at 5:46 PM, Michael Niedermayer 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 >> 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 >> >> --- >> >> 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 > From 8de26af99003646f3abfb8ee74d5d0bec7d20d08 Mon Sep 17 00:00:00 2001 From: Jacob Trimble Date: Tue, 30 Jan 2018 11:14:35 -0800 Subject: [PATCH] avformat/mov: Remove old encryption info methods. Signed-off-by: Jacob Trimble --- 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