[FFmpeg-devel,v5,1/4] libavcodec/dnxuc_parser: DNxUncompressed essence parser

This time only small corrections...

---
 libavcodec/dnxuc_parser.c | 124 ++++++++++++++++++++++++++++++++++++++
 libavcodec/parsers.c      |   1 +
 2 files changed, 125 insertions(+)
 create mode 100644 libavcodec/dnxuc_parser.c

On Wed, 11 Sep 2024, Martin Schitter wrote:

> This time only small corrections...

The order of the patches is wrong. Every point in a series must be 
compilable, and a patch in a series must only depend on earlier patches in 
the series.

So as a first patch you should add codec ID and codec desc, then you can 
add MXF demuxer support, then the parser and the decoder. And when adding 
the parser/decoder, also make the needed changes in the Makefile, not just 
as a separate patch.

Thanks,
Marton

>
> ---
> libavcodec/dnxuc_parser.c | 124 ++++++++++++++++++++++++++++++++++++++
> libavcodec/parsers.c      |   1 +
> 2 files changed, 125 insertions(+)
> create mode 100644 libavcodec/dnxuc_parser.c
>
> diff --git a/libavcodec/dnxuc_parser.c b/libavcodec/dnxuc_parser.c
> new file mode 100644
> index 0000000..55d5763
> --- /dev/null
> +++ b/libavcodec/dnxuc_parser.c
> @@ -0,0 +1,124 @@
> +/*
> + * Avid DNxUncomressed / SMPTE RDD 50 parser
> + * Copyright (c) 2024 Martin Schitter
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/*
> + This parser for DNxUncompressed video data is mostly based on
> + reverse engineering of output generated by DaVinci Resolve 19
> + but was later also checked against the SMPTE RDD 50 specification.
> +
> + Limitations: Multiple image planes are not supported.
> +*/
> +
> +#include "avcodec.h"
> +#include "libavutil/intreadwrite.h"
> +
> +typedef struct DNxUcParseContext {
> +    uint32_t fourcc_tag;
> +    uint32_t width;
> +    uint32_t height;
> +    uint32_t nr_bytes;
> +} DNxUcParseContext;
> +
> +/*
> +DNxUncompressed frame data comes wrapped in nested boxes of metadata
> +(box structure: len + fourcc marker + data):
> +
> +[0-4]   len of outer essence unit box (typically 37 bytes of header + frame data)
> +[4-7]   fourcc 'pack'
> +
> +[8-11]  len of "signal info box" (always 21)
> +[12-15] fourcc 'sinf'
> +[16-19] frame width / line packing size
> +[20-23] frame hight / nr of lines
> +[24-27] fourcc pixel format indicator
> +[28]    frame_layout (0: progressive, 1: interlaced)
> +
> +[29-32] len of "signal data box" (nr of frame data bytes + 8)
> +[33-36] fourcc 'sdat'
> +[37-..] frame data
> +
> +A sequence of 'signal info'+'signal data' box pairs wrapped in
> +'icmp'(=image component) boxes can be utilized to compose more
> +complex multi plane images.
> +This feature is only partially supported in the present implementation.
> +We never pick more than the first pair of info and image data enclosed
> +in this way.
> +*/
> +
> +static int dnxuc_parse(AVCodecParserContext *s,
> +                    AVCodecContext *avctx,
> +                    const uint8_t **poutbuf, int *poutbuf_size,
> +                    const uint8_t *buf, int buf_size)
> +{
> +    char fourcc_buf[5];
> +    const int HEADER_SIZE = 37;
> +    int icmp_offset = 0;
> +
> +    DNxUcParseContext *pc;
> +    pc = (DNxUcParseContext *) s->priv_data;
> +
> +    if (!buf_size) {
> +        return 0;
> +    }
> +    if (buf_size > 16 && MKTAG('i','c','m','p') == AV_RL32(buf+12)){
> +        icmp_offset += 8;
> +    }
> +    if ( buf_size < 37 + icmp_offset /* check metadata structure expectations */
> +        || MKTAG('p','a','c','k') != AV_RL32(buf+4+icmp_offset)
> +        || MKTAG('s','i','n','f') != AV_RL32(buf+12+icmp_offset)
> +        || MKTAG('s','d','a','t') != AV_RL32(buf+33+icmp_offset)){
> +            av_log(avctx, AV_LOG_ERROR, "can't read DNxUncompressed metadata.\n");
> +            *poutbuf_size = 0;
> +            return buf_size;
> +    }
> +
> +    pc->fourcc_tag = AV_RL32(buf+24+icmp_offset);
> +    pc->width = AV_RL32(buf+16+icmp_offset);
> +    pc->height = AV_RL32(buf+20+icmp_offset);
> +    pc->nr_bytes = AV_RL32(buf+29+icmp_offset) - 8;
> +
> +    if (!avctx->codec_tag) {
> +        av_fourcc_make_string(fourcc_buf, pc->fourcc_tag);
> +        av_log(avctx, AV_LOG_INFO, "dnxuc_parser: '%s' %dx%d %dbpp %d\n",
> +            fourcc_buf,
> +            pc->width, pc->height,
> +            (pc->nr_bytes*8)/(pc->width*pc->height),
> +            pc->nr_bytes);
> +        avctx->codec_tag = pc->fourcc_tag;
> +    }
> +
> +    if (pc->nr_bytes > buf_size - HEADER_SIZE + icmp_offset){
> +        av_log(avctx, AV_LOG_ERROR, "Insufficient size of image essence data.\n");
> +        *poutbuf_size = 0;
> +        return buf_size;
> +    }
> +
> +    *poutbuf = buf + HEADER_SIZE + icmp_offset;
> +    *poutbuf_size = pc->nr_bytes;
> +
> +    return buf_size;
> +}
> +
> +const AVCodecParser ff_dnxuc_parser = {
> +    .codec_ids      = { AV_CODEC_ID_DNXUC },
> +    .priv_data_size = sizeof(DNxUcParseContext),
> +    .parser_parse   = dnxuc_parse,
> +};
> diff --git a/libavcodec/parsers.c b/libavcodec/parsers.c
> index 5128009..8bfd2db 100644
> --- a/libavcodec/parsers.c
> +++ b/libavcodec/parsers.c
> @@ -35,6 +35,7 @@ extern const AVCodecParser ff_cri_parser;
> extern const AVCodecParser ff_dca_parser;
> extern const AVCodecParser ff_dirac_parser;
> extern const AVCodecParser ff_dnxhd_parser;
> +extern const AVCodecParser ff_dnxuc_parser;
> extern const AVCodecParser ff_dolby_e_parser;
> extern const AVCodecParser ff_dpx_parser;
> extern const AVCodecParser ff_dvaudio_parser;
> -- 
> 2.45.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On 11.09.24 21:47, Marton Balint wrote:
> The order of the patches is wrong. Every point in a series must be 
> compilable, and a patch in a series must only depend on earlier patches 
> in the series.
> 
> So as a first patch you should add codec ID and codec desc, then you can 
> add MXF demuxer support, then the parser and the decoder. And when 
> adding the parser/decoder, also make the needed changes in the Makefile, 
> not just as a separate patch.

You're right! I already recognized this defect yesterday when I saw the 
patchwork results. :(

I'm really unhappy about all this troubles related to git jugging and 
commit history modification.

On one hand I understand the demand for better reviewable small chunks 
of code -- and really appreciate all enormous valuable feedback here on 
the list! --, but I also see the inevitable drawbacks of this kind of 
workflow.

I'm rather sure, that nobody had recognized, that beside tiny fixes and 
rearrangements of this last patch set one file simple got lost.

I therefore personally prefer to use git version control not so much as 
a utility to create fancy code rearrangements and cosmetic cleanup, but 
as tool to control immutable history and traceable actual code changes, 
which it was made for.

In case of horrible sloppy developers like me, this may even uncover and 
blame questionable professional qualities by just unforgiving document 
all this frequently corrected typos and ugly glitches. But at the end 
it's still a rather useful instrument to correct and overcome exactly 
this kind of flaws.

I'm therefor unsure, if I'll really follow your advice and waste even 
more time on this error-prone code juggling game or just squash all the 
changes again to a reliable working bundle satisfying the final 
dependencies?

These more structured little code components, which will always compile 
if applied one after the other, may indeed look useful and seducing, but 
they would have to work even if applied in reverse order if they really 
represent individual pickable entities.

Well -- I'll see what I can do to make everybody happy...

Martin