[FFmpeg-devel] [PATCH] avformat/utils: Don't parse encrypted packets.

[Michael Niedermayer]

On Mon, Sep 17, 2018 at 02:35:44PM -0700, Jacob Trimble wrote:
> On Wed, Sep 12, 2018 at 11:50 AM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> >
> > On Tue, Sep 11, 2018 at 03:50:57PM -0700, Jacob Trimble wrote:
> > > [...]
> > >
> > > So how about, when we see an encrypted frame, we flush the parser
> > > before skipping the frame?  Can we just flush the parser and then
> > > reuse it later?  Or would we need to create a new parser if we saw
> > > clear frames later?
> >
> > try/test it and review the code. only way to know for sure what works
> >
> > thanks
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > The bravest are surely those who have the clearest vision
> > of what is before them, glory and danger alike, and yet
> > notwithstanding go out to meet it. -- Thucydides
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> Here is a new patch that flushes the parser before returning the
> encrypted packets.  It also logs when it does this.

>  utils.c |   41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> ef62a54a0acee61029b78e294a030f416785b64a  0001-avformat-utils-Don-t-parse-encrypted-packets-v2.patch
> From 3955dad8070c28b021afc3304939500a09c86fcd Mon Sep 17 00:00:00 2001
> From: Jacob Trimble <modmaker at google.com>
> Date: Tue, 28 Aug 2018 10:40:29 -0700
> Subject: [PATCH] avformat/utils: Don't parse encrypted packets.
> 
> If a packet is full-sample encrypted, then packet data can't be parsed
> without decrypting it.  So this skips the packet parsing for those
> packets.  If the packet has sub-sample encryption, it is assumed that
> the headers are in the clear and the parser will only need that info.
> 
> Signed-off-by: Jacob Trimble <modmaker at google.com>
> ---
>  libavformat/utils.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index a72f0a482e..8d84674797 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -27,6 +27,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/dict.h"
> +#include "libavutil/encryption_info.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/opt.h"
> @@ -1576,6 +1577,9 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>      while (!got_packet && !s->internal->parse_queue) {
>          AVStream *st;
>          AVPacket cur_pkt;
> +        uint8_t *enc_side_data;
> +        int enc_side_data_size;
> +        int is_full_encrypted = 0;
>  
>          /* read next packet */
>          ret = ff_read_packet(s, &cur_pkt);
> @@ -1643,7 +1647,23 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                     av_ts2str(cur_pkt.dts),
>                     cur_pkt.size, cur_pkt.duration, cur_pkt.flags);
>  
> -        if (st->need_parsing && !st->parser && !(s->flags & AVFMT_FLAG_NOPARSE)) {
> +        /* if the packet is full-sample encrypted, we can't parse it.  If the
> +         * packet uses sub-sample encryption, assume the headers are clear and
> +         * can still be parsed.
> +         */
> +        enc_side_data = av_packet_get_side_data(
> +            &cur_pkt, AV_PKT_DATA_ENCRYPTION_INFO, &enc_side_data_size);
> +        if (enc_side_data) {
> +            AVEncryptionInfo *enc_info =
> +                av_encryption_info_get_side_data(enc_side_data, enc_side_data_size);
> +            if (enc_info) {
> +                is_full_encrypted = enc_info->subsample_count == 0;
> +                av_encryption_info_free(enc_info);
> +            }
> +        }

I dont think that a allocation and deallocation for basically checking one bit
is ideal. This is done for every packet and there can be many small packets.



> +
> +        if (st->need_parsing && !st->parser && !(s->flags & AVFMT_FLAG_NOPARSE) &&
> +            !is_full_encrypted) {
>              st->parser = av_parser_init(st->codecpar->codec_id);
>              if (!st->parser) {
>                  av_log(s, AV_LOG_VERBOSE, "parser not found for codec "
> @@ -1659,6 +1679,25 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                  st->parser->flags |= PARSER_FLAG_USE_CODEC_TS;
>          }
>  
> +        if (st->parser && is_full_encrypted) {
> +            av_log(s, AV_LOG_VERBOSE, "skipping parsing of encrypted packets.\n");
> +
> +            /* flush any packets from the parser. */
> +            parse_packet(s, NULL, cur_pkt.stream_index);
> +            if (s->internal->parse_queue) {
> +                /* if we have other packets, append this packet to the end and read
> +                 * from the queue instead. */
> +                compute_pkt_fields(s, st, NULL, &cur_pkt, AV_NOPTS_VALUE, AV_NOPTS_VALUE);
> +                ret = ff_packet_list_put(&s->internal->parse_queue,
> +                                         &s->internal->parse_queue_end,
> +                                         &cur_pkt, 0);
> +                if (ret < 0)
> +                    return ret;
> +                break;
> +            }
> +            av_assert2(!st->parser);
> +        }

have you tested streams that mix encrypted and unencrypted packets?
I mean has this parser on/off code been tested ?

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181024/2cfc23a4/attachment.sig>