> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".