diff mbox

[FFmpeg-devel,v2,3/3] lavc/hevc_mp4toannexb: Exit earlier if there is no nalu payload remaining

Message ID 20191204000411.29765-3-andriy.gelman@gmail.com
State New
Headers show

Commit Message

Andriy Gelman Dec. 4, 2019, 12:04 a.m. UTC
From: Andriy Gelman <andriy.gelman@gmail.com>

Since the nal unit payload is located after the length prefix, there is
no reason to continue reading if there is no nal unit payload remaining.
---
 libavcodec/hevc_mp4toannexb_bsf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt Dec. 4, 2019, 12:39 a.m. UTC | #1
On Wed, Dec 4, 2019 at 1:04 AM Andriy Gelman <andriy.gelman@gmail.com>
wrote:

> From: Andriy Gelman <andriy.gelman@gmail.com>
>
> Since the nal unit payload is located after the length prefix, there is
> no reason to continue reading if there is no nal unit payload remaining.
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> b/libavcodec/hevc_mp4toannexb_bsf.c
> index faf516634b2..c0968b63239 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -136,7 +136,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *out)
>
>      bytestream2_init(&gb, in->data, in->size);
>
> -    while (bytestream2_get_bytes_left(&gb)) {
> +    while (bytestream2_get_bytes_left(&gb) > s->length_size) {
>

So if there is some data left, you simply ignore it. It could also be
argued that this is invalid data.
(The packet you are dealing with here is padded btw, so you don't need to
worry about reading beyond the end when reading the size field; which
actually can't happen anyway, because the safe version of the bytestream
API is used throughout.)


>          uint32_t nalu_size = 0;
>          int      nalu_type;
>          int is_irap, add_extradata, extra_size, prev_size;
> --
> 2.24.0
>
Andriy Gelman Dec. 4, 2019, 2:59 a.m. UTC | #2
On Wed, 04. Dec 01:39, Andreas Rheinhardt wrote:
> On Wed, Dec 4, 2019 at 1:04 AM Andriy Gelman <andriy.gelman@gmail.com>
> wrote:
> 
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> >
> > Since the nal unit payload is located after the length prefix, there is
> > no reason to continue reading if there is no nal unit payload remaining.
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> > b/libavcodec/hevc_mp4toannexb_bsf.c
> > index faf516634b2..c0968b63239 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -136,7 +136,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> > AVPacket *out)
> >
> >      bytestream2_init(&gb, in->data, in->size);
> >
> > -    while (bytestream2_get_bytes_left(&gb)) {
> > +    while (bytestream2_get_bytes_left(&gb) > s->length_size) {
> >
> 
> So if there is some data left, you simply ignore it. It could also be
> argued that this is invalid data.
> (The packet you are dealing with here is padded btw, so you don't need to
> worry about reading beyond the end when reading the size field; which
> actually can't happen anyway, because the safe version of the bytestream
> API is used throughout.)

I suppose it would be invalid data. 

If there are <= s->length_size bytes left then there is no nal unit payload and
there is no point to even parse nalu_size. Plus you avoid calling av_grow_packet() an
extra time.

Thanks,
diff mbox

Patch

diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
index faf516634b2..c0968b63239 100644
--- a/libavcodec/hevc_mp4toannexb_bsf.c
+++ b/libavcodec/hevc_mp4toannexb_bsf.c
@@ -136,7 +136,7 @@  static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
 
     bytestream2_init(&gb, in->data, in->size);
 
-    while (bytestream2_get_bytes_left(&gb)) {
+    while (bytestream2_get_bytes_left(&gb) > s->length_size) {
         uint32_t nalu_size = 0;
         int      nalu_type;
         int is_irap, add_extradata, extra_size, prev_size;