Re: [FFmpeg-devel] [PATCH] lavu: make av_get_media_type_string() never return NULL

From: Scott Theisen <scott.the.elm@gmail.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH] lavu: make av_get_media_type_string() never return NULL
Date: Tue, 1 Feb 2022 21:42:32 -0500
Message-ID: <52297cd1-a76b-5224-8dcc-e275edc0b528@gmail.com> (raw)
In-Reply-To: <5653ed95-8318-d383-d2bd-bf60535743a8@gmail.com>

On 2/1/22 21:06, James Almer wrote:
>> 32/44 uses do not check for null.
>
> But do they need to? Those modules have probably ensured media type is 
> one of the known and supported ones by the time they call this function.
>
> Why do you need this function to never return NULL, anyway? If you 
> know any of these calls where media type can be UNKNOWN, then it's 
> easier to just fix them by checking for NULL as per the doxy. 
The impetus was the desire to use this function in MythTV without 
invoking undefined behavior.  Currently MythTV uses a modification to 
ffmpeg: 
https://github.com/MythTV/mythtv/blob/7fa02d4dc3ab0d3f2cbfcbc514047fe2736fe9cf/mythtv/external/FFmpeg/libavcodec/utils-mythtv.c#L228
(For reference, I just tested and a NULL/nullptr const char* becomes an 
empty QString, but this is not documented.)

Also, another reason was to match behavior with avcodec_get_name() which 
never returns NULL, instead it returns "unknown_codec".

On 2/1/22 21:13, Andreas Rheinhardt wrote:
> That would need an announcement in doc/APIchanges; it can then be
> changed at the next major version bump after two years have passed.
So, would creating a new function instead and deprecating the old one be 
a better option?  (e.g. av_get_media_type_name)
> Who sets invalid media types?
> (In case of fftools/*: If they don't set it themselves, they (and all
> our users) should be able to rely on our libraries to not set invalid
> values. The same goes for all demuxers (as they set this value
> themselves). Checks are only necessary where the media type comes from
> the user; this means muxers (where the check should be done generically)
> and possibly avfiltergraph.c (I am pretty sure that the type has already
> been checked earlier for filters).)
I agree invalid media types are probably not set by anyone, but without 
checking for non-NULL it potentially invokes undefined behavior.

Returning "unknown" for AVMEDIA_TYPE_UNKNOWN and returning a different 
value for the default case is, in my opinion clearer. 
"invalid_media_type" may be a better default string to match the 
behavior of avcodec_get_name().

Regards,
Scott
_______________________________________________
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".