| Message ID | 1561640354-12545-2-git-send-email-zhong.li@intel.com |
|---|---|
| State | New |
| Headers | show |
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
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
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/ [...]
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.
> 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?)
> 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.
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, };
Signed-off-by: Zhong Li <zhong.li@intel.com> --- libavcodec/mjpeg_parser.c | 158 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 157 insertions(+), 1 deletion(-)