diff mbox

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

Message ID CAO7y9i_f81CmwqL8PgwGoQb=fYqDHFBVycWDrKG5ijC5vPRNvA@mail.gmail.com
State New
Headers show

Commit Message

Jacob Trimble Sept. 17, 2018, 9:35 p.m. UTC 
On Wed, Sep 12, 2018 at 11:50 AM Michael Niedermayer
<michael@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@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.

Comments

Jacob Trimble Oct. 1, 2018, 6:30 p.m. UTC | #1 
On Mon, Sep 17, 2018 at 2:35 PM Jacob Trimble <modmaker@google.com> wrote:
>
> On Wed, Sep 12, 2018 at 11:50 AM Michael Niedermayer
> <michael@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@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.

Ping.  Is this better?
Jacob Trimble Oct. 23, 2018, 4:43 p.m. UTC | #2 
On Mon, Oct 1, 2018 at 11:30 AM Jacob Trimble <modmaker@google.com> wrote:
>
> On Mon, Sep 17, 2018 at 2:35 PM Jacob Trimble <modmaker@google.com> wrote:
> >
> > On Wed, Sep 12, 2018 at 11:50 AM Michael Niedermayer
> > <michael@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@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.
>
> Ping.  Is this better?

Ping.
Michael Niedermayer Oct. 24, 2018, 5:12 p.m. UTC | #3 
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@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@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@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@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 ?

[...]
diff mbox

Patch

From 3955dad8070c28b021afc3304939500a09c86fcd Mon Sep 17 00:00:00 2001
From: Jacob Trimble <modmaker@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@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);
+            }
+        }
+
+        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);
+        }
+
         if (!st->need_parsing || !st->parser) {
             /* no parsing needed: we just output the packet as is */
             *pkt = cur_pkt;
-- 
2.19.0.397.gdd90340f6a-goog