| Message ID | CAO7y9i_4Xfi3psvt3JacnRHREBY73pK6DJKqE4grF8-L1wkh+Q@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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.
On Mon, Feb 12, 2018 at 9:35 AM, Jacob Trimble <modmaker@google.com> wrote: > 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. Ping again. I know this is low priority, but I would like to get these merged soon.
On Mon, Mar 5, 2018 at 12:22 PM, Jacob Trimble <modmaker@google.com> wrote: > On Mon, Feb 12, 2018 at 9:35 AM, Jacob Trimble <modmaker@google.com> wrote: >> 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. > > 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.
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