[FFmpeg-devel,2/3] lavc/mjpeg_parser: use ff_mjpeg_decode_header to parse frame info

Submitted by Zhong Li on June 27, 2019, 12:59 p.m.

Details

Message ID 1561640354-12545-2-git-send-email-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li June 27, 2019, 12:59 p.m.
Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/mjpeg_parser.c | 158 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos June 27, 2019, 7:05 p.m.
Am Do., 27. Juni 2019 um 14:59 Uhr schrieb Zhong Li <zhong.li@intel.com>:
>
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/mjpeg_parser.c | 158 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/mjpeg_parser.c b/libavcodec/mjpeg_parser.c
> index 07a6b2b..f59aa3e 100644
> --- a/libavcodec/mjpeg_parser.c
> +++ b/libavcodec/mjpeg_parser.c
> @@ -27,12 +27,131 @@
>   */
>
>  #include "parser.h"
> +#include "mjpeg.h"
> +#include "mjpegdec.h"
> +#include "get_bits.h"
>
>  typedef struct MJPEGParserContext{
>      ParseContext pc;
> +    MJpegDecodeContext dec_ctx;
>      int size;
>  }MJPEGParserContext;
>
> +/* return the 8 bit start code value and update the search
> +   state. Return -1 if no start code found */
> +static int find_frame_header_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end)
> +{
> +    const uint8_t *buf_ptr;
> +    unsigned int v, v2;
> +    int val;
> +    int skipped = 0;
> +
> +    buf_ptr = *pbuf_ptr;
> +    while (buf_end - buf_ptr > 1) {
> +        v  = *buf_ptr++;
> +        v2 = *buf_ptr;

> +        if ((v == 0xff) && buf_ptr < buf_end &&
> +            ((v2 >= SOF0) && (v2 <= SOF3)) ) {

It may be just me but there are too many parentheses in these
lines (and not enough braces in the rest of the patch).

> +            val = *buf_ptr++;
> +            goto found;
> +        }
> +        skipped++;
> +    }
> +    buf_ptr = buf_end;
> +    val = -1;
> +found:
> +    ff_dlog(NULL, "find_marker skipped %d bytes\n", skipped);
> +    *pbuf_ptr = buf_ptr;
> +    return val;
> +}
> +
> +static void jpeg_set_interlace_polarity(AVCodecContext *avctx, MJpegDecodeContext *dec_ctx)
> +{
> +    if (avctx->extradata_size > 14
> +        && AV_RL32(avctx->extradata) == 0x2C
> +        && AV_RL32(avctx->extradata+4) == 0x18) {
> +        if (avctx->extradata[12] == 1) /* NTSC */
> +            dec_ctx->interlace_polarity = 1;
> +        if (avctx->extradata[12] == 2) /* PAL */
> +            dec_ctx->interlace_polarity = 0;
> +    }
> +}
> +
> +static int jpeg_parse_header(AVCodecParserContext *s, AVCodecContext *avctx,
> +                               const uint8_t *buf, int buf_size)
> +{
> +    MJPEGParserContext *m = s->priv_data;
> +    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
> +    int start_code;
> +    const uint8_t *start, *end;
> +    int ret=0;
> +
> +    start = buf;
> +    end = buf + buf_size;
> +    start_code = find_frame_header_marker(&start, end);
> +    if (start_code < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "parse header failure:"
> +            "can't find supported marker type (%x)\n", start_code);
> +
> +        return -1;
> +    } else
> +        av_log(avctx, AV_LOG_DEBUG, "marker=%x\n", start_code);
> +
> +    jpeg_set_interlace_polarity(avctx, dec_ctx);
> +    init_get_bits8(&dec_ctx->gb, start, end - start);
> +    dec_ctx->avctx = avctx;
> +
> +    switch(start_code) {
> +    case SOF0:
> +        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT;
> +        dec_ctx->lossless        = 0;
> +        dec_ctx->progressive     = 0;
> +        break;
> +    case SOF1:
> +        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT;
> +        dec_ctx->lossless        = 0;
> +        dec_ctx->progressive     = 0;
> +        break;
> +    case SOF2:
> +        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT;
> +        dec_ctx->lossless        = 0;
> +        dec_ctx->progressive     = 1;
> +        break;
> +    case SOF3:
> +        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS;
> +        dec_ctx->lossless        = 1;
> +        dec_ctx->progressive     = 0;
> +        break;
> +    default:
> +       assert(0);
> +    }
> +
> +    ret = ff_mjpeg_decode_header(dec_ctx);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_WARNING, "Failed to parse header\n");
> +        return ret;
> +    }
> +
> +    s->height = dec_ctx->height;
> +    s->width  = dec_ctx->width;
> +    s->coded_height = s->height;
> +    s->coded_width  = s->width;
> +    s->format       = avctx->pix_fmt;
> +    s->pict_type    = AV_PICTURE_TYPE_I;
> +    s->key_frame    = 1;
> +
> +    if (dec_ctx->interlaced) {
> +        if (dec_ctx->bottom_field)
> +            s->field_order = AV_FIELD_BB;
> +        else
> +            s->field_order = AV_FIELD_TT;
> +    } else
> +        s->field_order = AV_FIELD_PROGRESSIVE;
> +
> +    return 0;
> +}
> +
> +
>  /**
>   * Find the end of the current frame in the bitstream.
>   * @return the position of the first byte of the next frame, or -1
> @@ -99,6 +218,40 @@ static int find_frame_end(MJPEGParserContext *m, const uint8_t *buf, int buf_siz
>      return END_NOT_FOUND;
>  }
>
> +static av_cold int jpeg_parse_init(AVCodecParserContext *s)
> +{
> +    MJPEGParserContext *m = s->priv_data;
> +    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
> +
> +    if (!dec_ctx->picture_ptr) {
> +        dec_ctx->picture = av_frame_alloc();
> +        if (!dec_ctx->picture)
> +            return AVERROR(ENOMEM);
> +        dec_ctx->picture_ptr = dec_ctx->picture;
> +    }
> +
> +    dec_ctx->first_picture = 1;
> +    dec_ctx->got_picture   = 0;
> +    dec_ctx->org_height    = 0;
> +    dec_ctx->ls            = 0;
> +    return 0;
> +}
> +
> +static av_cold void jpeg_parse_close(AVCodecParserContext *s)
> +{
> +    MJPEGParserContext *m = s->priv_data;
> +    ParseContext *pc = &m->pc;
> +    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
> +
> +    av_freep(&pc->buffer);
> +
> +    if (dec_ctx->picture) {
> +        av_frame_free(&dec_ctx->picture);
> +        dec_ctx->picture_ptr = NULL;
> +    } else if (dec_ctx->picture_ptr)
> +        av_frame_unref(dec_ctx->picture_ptr);
> +}
> +
>  static int jpeg_parse(AVCodecParserContext *s,
>                        AVCodecContext *avctx,
>                        const uint8_t **poutbuf, int *poutbuf_size,
> @@ -120,6 +273,8 @@ static int jpeg_parse(AVCodecParserContext *s,
>          }
>      }
>
> +    jpeg_parse_header(s, avctx, buf, buf_size);

Does this patch have any effect (performance or otherwise) on
software decoding?

Carl Eugen
Carl Eugen Hoyos June 27, 2019, 7:13 p.m.
Am Do., 27. Juni 2019 um 14:59 Uhr schrieb Zhong Li <zhong.li@intel.com>:

> +/* return the 8 bit start code value and update the search
> +   state. Return -1 if no start code found */
> +static int find_frame_header_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end)
> +{
> +    const uint8_t *buf_ptr;
> +    unsigned int v, v2;
> +    int val;
> +    int skipped = 0;
> +
> +    buf_ptr = *pbuf_ptr;
> +    while (buf_end - buf_ptr > 1) {
> +        v  = *buf_ptr++;
> +        v2 = *buf_ptr;
> +        if ((v == 0xff) && buf_ptr < buf_end &&
> +            ((v2 >= SOF0) && (v2 <= SOF3)) ) {
> +            val = *buf_ptr++;
> +            goto found;
> +        }
> +        skipped++;
> +    }
> +    buf_ptr = buf_end;
> +    val = -1;
> +found:
> +    ff_dlog(NULL, "find_marker skipped %d bytes\n", skipped);
> +    *pbuf_ptr = buf_ptr;
> +    return val;
> +}

Sorry for not realizing this earlier (I searched for "SOF0"):
Why is this function duplicated?

Carl Eugen
Michael Niedermayer June 28, 2019, 8:11 a.m.
On Thu, Jun 27, 2019 at 08:59:13PM +0800, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/mjpeg_parser.c | 158 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)

this breaks mjpeg decoding

for example tickets/3229/bad.avi
https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3229/

[...]
James Almer June 28, 2019, 4:56 p.m.
On 6/27/2019 9:59 AM, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/mjpeg_parser.c | 158 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mjpeg_parser.c b/libavcodec/mjpeg_parser.c
> index 07a6b2b..f59aa3e 100644
> --- a/libavcodec/mjpeg_parser.c
> +++ b/libavcodec/mjpeg_parser.c
> @@ -27,12 +27,131 @@
>   */
>  
>  #include "parser.h"
> +#include "mjpeg.h"
> +#include "mjpegdec.h"
> +#include "get_bits.h"
>  
>  typedef struct MJPEGParserContext{
>      ParseContext pc;
> +    MJpegDecodeContext dec_ctx;

This is not acceptable. The parser shouldn't use decoder structs, as it
makes the former depend on the latter.

A reusable header parsing function should be standalone, so it may be
called from decoders, parsers, bitstream filters, and anything that may
require it.
Zhong Li July 1, 2019, 2:35 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Friday, June 28, 2019 4:12 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/3] lavc/mjpeg_parser: use
> ff_mjpeg_decode_header to parse frame info
> 
> On Thu, Jun 27, 2019 at 08:59:13PM +0800, Zhong Li wrote:
> > Signed-off-by: Zhong Li <zhong.li@intel.com>
> > ---
> >  libavcodec/mjpeg_parser.c | 158
> > +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 157 insertions(+), 1 deletion(-)
> 
> this breaks mjpeg decoding
> 
> for example tickets/3229/bad.avi
> https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3229/

Ok, will fix and update.
(Probably adding this case to fate is a good idea?)
Zhong Li July 1, 2019, 3:36 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of James Almer

> Sent: Saturday, June 29, 2019 12:56 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 2/3] lavc/mjpeg_parser: use

> ff_mjpeg_decode_header to parse frame info

> 

> On 6/27/2019 9:59 AM, Zhong Li wrote:

> > Signed-off-by: Zhong Li <zhong.li@intel.com>

> > ---

> >  libavcodec/mjpeg_parser.c | 158

> > +++++++++++++++++++++++++++++++++++++++++++++-

> >  1 file changed, 157 insertions(+), 1 deletion(-)

> >

> > diff --git a/libavcodec/mjpeg_parser.c b/libavcodec/mjpeg_parser.c

> > index 07a6b2b..f59aa3e 100644

> > --- a/libavcodec/mjpeg_parser.c

> > +++ b/libavcodec/mjpeg_parser.c

> > @@ -27,12 +27,131 @@

> >   */

> >

> >  #include "parser.h"

> > +#include "mjpeg.h"

> > +#include "mjpegdec.h"

> > +#include "get_bits.h"

> >

> >  typedef struct MJPEGParserContext{

> >      ParseContext pc;

> > +    MJpegDecodeContext dec_ctx;

> 

> This is not acceptable. The parser shouldn't use decoder structs, as it makes

> the former depend on the latter.

> 

> A reusable header parsing function should be standalone, so it may be called

> from decoders, parsers, bitstream filters, and anything that may require it.


Thanks for your comment, James.
This is not the first time to introduce decoder dependency, please see mpeg4video_parser.c
And IMHO no matter what is the form you parse a header, actually it is a part of decoding process. 
Refusing to reuse decoder context or function will introduce huge code duplication and bring trouble to maintain later.

Probably set up a middle layer just like h264_ps.c for both decoder and parser to use is a better idea?
(But a tricky thing is that mjpeg header parser is a litter more complex h264, pix_fmt and color_range is not just desided by frame header makers, but also comment makers:
See:
        case 0x11111100:
            if (s->rgb)
                s->avctx->pix_fmt = s->bits <= 9 ? AV_PIX_FMT_BGR24 : AV_PIX_FMT_BGR48;
            else {
                if (   s->adobe_transform == 0
                    || s->component_id[0] == 'R' - 1 && s->component_id[1] == 'G' - 1 && s->component_id[2] == 'B' - 1) {
                    s->avctx->pix_fmt = s->bits <= 8 ? AV_PIX_FMT_GBRP : AV_PIX_FMT_GBRP16;
                } else {
                    if (s->bits <= 8) s->avctx->pix_fmt = s->cs_itu601 ? AV_PIX_FMT_YUV444P : AV_PIX_FMT_YUVJ444P;
                    else              s->avctx->pix_fmt = AV_PIX_FMT_YUV444P16;
                s->avctx->color_range = s->cs_itu601 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;
                }
            }

Here cs_itu601 is parsed from comment makers. 
)

Any suggestion/comment is welcome.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mjpeg_parser.c b/libavcodec/mjpeg_parser.c
index 07a6b2b..f59aa3e 100644
--- a/libavcodec/mjpeg_parser.c
+++ b/libavcodec/mjpeg_parser.c
@@ -27,12 +27,131 @@ 
  */
 
 #include "parser.h"
+#include "mjpeg.h"
+#include "mjpegdec.h"
+#include "get_bits.h"
 
 typedef struct MJPEGParserContext{
     ParseContext pc;
+    MJpegDecodeContext dec_ctx;
     int size;
 }MJPEGParserContext;
 
+/* return the 8 bit start code value and update the search
+   state. Return -1 if no start code found */
+static int find_frame_header_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end)
+{
+    const uint8_t *buf_ptr;
+    unsigned int v, v2;
+    int val;
+    int skipped = 0;
+
+    buf_ptr = *pbuf_ptr;
+    while (buf_end - buf_ptr > 1) {
+        v  = *buf_ptr++;
+        v2 = *buf_ptr;
+        if ((v == 0xff) && buf_ptr < buf_end &&
+            ((v2 >= SOF0) && (v2 <= SOF3)) ) {
+            val = *buf_ptr++;
+            goto found;
+        }
+        skipped++;
+    }
+    buf_ptr = buf_end;
+    val = -1;
+found:
+    ff_dlog(NULL, "find_marker skipped %d bytes\n", skipped);
+    *pbuf_ptr = buf_ptr;
+    return val;
+}
+
+static void jpeg_set_interlace_polarity(AVCodecContext *avctx, MJpegDecodeContext *dec_ctx)
+{
+    if (avctx->extradata_size > 14
+        && AV_RL32(avctx->extradata) == 0x2C
+        && AV_RL32(avctx->extradata+4) == 0x18) {
+        if (avctx->extradata[12] == 1) /* NTSC */
+            dec_ctx->interlace_polarity = 1;
+        if (avctx->extradata[12] == 2) /* PAL */
+            dec_ctx->interlace_polarity = 0;
+    }
+}
+
+static int jpeg_parse_header(AVCodecParserContext *s, AVCodecContext *avctx,
+                               const uint8_t *buf, int buf_size)
+{
+    MJPEGParserContext *m = s->priv_data;
+    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
+    int start_code;
+    const uint8_t *start, *end;
+    int ret=0;
+
+    start = buf;
+    end = buf + buf_size;
+    start_code = find_frame_header_marker(&start, end);
+    if (start_code < 0) {
+        av_log(avctx, AV_LOG_ERROR, "parse header failure:"
+            "can't find supported marker type (%x)\n", start_code);
+
+        return -1;
+    } else
+        av_log(avctx, AV_LOG_DEBUG, "marker=%x\n", start_code);
+
+    jpeg_set_interlace_polarity(avctx, dec_ctx);
+    init_get_bits8(&dec_ctx->gb, start, end - start);
+    dec_ctx->avctx = avctx;
+
+    switch(start_code) {
+    case SOF0:
+        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT;
+        dec_ctx->lossless        = 0;
+        dec_ctx->progressive     = 0;
+        break;
+    case SOF1:
+        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT;
+        dec_ctx->lossless        = 0;
+        dec_ctx->progressive     = 0;
+        break;
+    case SOF2:
+        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT;
+        dec_ctx->lossless        = 0;
+        dec_ctx->progressive     = 1;
+        break;
+    case SOF3:
+        avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS;
+        dec_ctx->lossless        = 1;
+        dec_ctx->progressive     = 0;
+        break;
+    default:
+       assert(0);
+    }
+
+    ret = ff_mjpeg_decode_header(dec_ctx);
+    if (ret < 0) {
+        av_log(avctx, AV_LOG_WARNING, "Failed to parse header\n");
+        return ret;
+    }
+
+    s->height = dec_ctx->height;
+    s->width  = dec_ctx->width;
+    s->coded_height = s->height;
+    s->coded_width  = s->width;
+    s->format       = avctx->pix_fmt;
+    s->pict_type    = AV_PICTURE_TYPE_I;
+    s->key_frame    = 1;
+
+    if (dec_ctx->interlaced) {
+        if (dec_ctx->bottom_field)
+            s->field_order = AV_FIELD_BB;
+        else
+            s->field_order = AV_FIELD_TT;
+    } else
+        s->field_order = AV_FIELD_PROGRESSIVE;
+
+    return 0;
+}
+
+
 /**
  * Find the end of the current frame in the bitstream.
  * @return the position of the first byte of the next frame, or -1
@@ -99,6 +218,40 @@  static int find_frame_end(MJPEGParserContext *m, const uint8_t *buf, int buf_siz
     return END_NOT_FOUND;
 }
 
+static av_cold int jpeg_parse_init(AVCodecParserContext *s)
+{
+    MJPEGParserContext *m = s->priv_data;
+    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
+
+    if (!dec_ctx->picture_ptr) {
+        dec_ctx->picture = av_frame_alloc();
+        if (!dec_ctx->picture)
+            return AVERROR(ENOMEM);
+        dec_ctx->picture_ptr = dec_ctx->picture;
+    }
+
+    dec_ctx->first_picture = 1;
+    dec_ctx->got_picture   = 0;
+    dec_ctx->org_height    = 0;
+    dec_ctx->ls            = 0;
+    return 0;
+}
+
+static av_cold void jpeg_parse_close(AVCodecParserContext *s)
+{
+    MJPEGParserContext *m = s->priv_data;
+    ParseContext *pc = &m->pc;
+    MJpegDecodeContext *dec_ctx = &m->dec_ctx;
+
+    av_freep(&pc->buffer);
+
+    if (dec_ctx->picture) {
+        av_frame_free(&dec_ctx->picture);
+        dec_ctx->picture_ptr = NULL;
+    } else if (dec_ctx->picture_ptr)
+        av_frame_unref(dec_ctx->picture_ptr);
+}
+
 static int jpeg_parse(AVCodecParserContext *s,
                       AVCodecContext *avctx,
                       const uint8_t **poutbuf, int *poutbuf_size,
@@ -120,6 +273,8 @@  static int jpeg_parse(AVCodecParserContext *s,
         }
     }
 
+    jpeg_parse_header(s, avctx, buf, buf_size);
+
     *poutbuf = buf;
     *poutbuf_size = buf_size;
     return next;
@@ -129,6 +284,7 @@  static int jpeg_parse(AVCodecParserContext *s,
 AVCodecParser ff_mjpeg_parser = {
     .codec_ids      = { AV_CODEC_ID_MJPEG, AV_CODEC_ID_JPEGLS },
     .priv_data_size = sizeof(MJPEGParserContext),
+    .parser_init      = jpeg_parse_init,
     .parser_parse   = jpeg_parse,
-    .parser_close   = ff_parse_close,
+    .parser_close   = jpeg_parse_close,
 };