[FFmpeg-devel] [PATCH v7 1/2] avformat/imf: Demuxer

[Pierre-Anthony Lemieux]

On Mon, Dec 6, 2021 at 9:45 AM Lynne <dev at lynne.ee> wrote:
>
> 6 Dec 2021, 04:18 by pal at sandflow.com:
>
> > From: Pierre-Anthony Lemieux <pal at palemieux.com>
> > +
> > +/**
>
> Please don't NIH-parse UUIDs. They're more complicated
> than this. Use libuuid, it's stable, bug-free and universally
> available. I submitted a patch for it last month for an
> unrelated use, which I decided to drop, but you
> can see how it was done there.

libuuid might be overkill and not a great match in the case of the IMF demuxer:

- libuuid does not parse the URN-encoding of UUIDs, which is used in
the IMF CPL ("urn:uuid:017b57a3-c61e-4b50-8c62-4586ea8a7769")
- the IMF demuxer does not need to generate UUIDs
- the IMF demuxer does not depend about the internal fields/format of
UUIDs or need to manipulate UUIDs: it merely compares them
byte-by-byte

>
> Also, please follow the code style. We use snake_case
> for function names, which means there are no capital
> letters in them.

Addressed in patch v8.

> There are also still one or two one-line statements
> left that are wrapped in brackets.

Addressed in patch v8.

>
> We check every single allocation we make, since malloc()
> can fail on Windows. So that means all av_strdup calls,
> av_append_path_component calls and so on can
> return NULL, and result in a nasty crash.

Addressed in patch v8.

>
> Do not use strtok. It's not thread-safe at all. Use av_strtok,
> which is thread-safe.

Addressed in patch v8.

>
> ff_imf_cpl_free() has a weird branch that can just be
> done with a return if cpl is NULL.

Addressed in patch v8.

>
> There are places where you call av_free() and then
> set the pointer to NULL. We have a convenience
> function for this, av_freep().

Addressed in patch v8.
>
> Please use defined-length integers instead of "unsigned long".
> If something needs to be 64-bits, use (u)int64_t. If something
> doesn't need to be 64-bits, use uint32_t for unsigned, or plain
> int for signed. We always require plain ints to be 32-bits.

Addressed in patch v8.

>
> Also, instead of printing 64-bit ints via "%lu", use "%"PRIu64"
> and "%"PRIi64". MSVC has problems with %lu.

Addressed in patch v8.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".