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

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of James Almer
> Sent: Saturday, June 29, 2019 12:56 AM
> To: ffmpeg-devel at 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 at 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.