[FFmpeg-devel] Should add AVProbeData change to API changes + release notes

[wm4]

On Fri, 12 Sep 2014 18:51:48 +0200
Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:

> Hi,
> 
> On 12.09.2014 15:18, Michael Niedermayer wrote:
> > On Fri, Sep 12, 2014 at 01:54:36PM +0200, Andreas Cadhalpun wrote:
> >> On 11.08.2014 22:22, Michael Niedermayer wrote:
> >>> On Mon, Aug 11, 2014 at 08:05:38PM +0200, Reimar Döffinger wrote:
> >>>> Hello,
> >>>> (sorry for being too lazy to send a patch)
> >>>> With the major version bump AVProbeData was extended by a new field.
> >>>> This so far has broken 3 places within FFmpeg and one within MPlayer,
> >>>> where AVProbeData was only initialized field-by-field.
> >>>> This will cause "random" crashes.
> >>>> I'm at this point fairly certain a lot of other software will have the
> >>>> same issue.
> >>
> >> That's for sure.
> >>
> >>>> I suggest we make add a big note with the release that everyone should
> >>>> check their software for uses of AVProbeData that might result in parts
> >>>> of that struct not being initialized.
> >>>
> >>> agree
> >>
> >> Please really document this!
> 
> Attached 0001 patch adds documentation for this.
> The 0002 patch changes the score to AVPROBE_SCORE_MIME for matching mime 
> types, which seems to have been intended [0].

I'm not very sure; note that AVPROBE_SCORE_EXTENSION does NOT refer to
the probe score set if the extension matches. This is a relict from the
past, and actual extension probe scores are much lower. But I guess
AVPROBE_SCORE_MIME was indeed to be used this way.

> >> It broke mpd [1] and the mpd developer wasn't happy that this is not
> >> documented [2].
> >>
> >> But there is also another problem, as can be seen from the comment
> >> in the fix [3]:
> >> /* this attribute was added in libav/ffmpeg version 11, but
> >>     unfortunately it's "uint8_t" instead of "char", and it's
> >>     not "const" - wtf? */
> >>
> >
> >> I'm also wondering, why AVProbeData.mime_type is a uint8_t* instead
> >> of a const char*.
> >
> > dont ask me
> 
> Well, then I'm CC'ing the author of this commit.
> 
> >> This was introduced in commit
> >> 3a19405d574a467c68b48e4b824c76617fd59de0 (merged from Libav) and in
> >> the same commit lpd.mime_type is used as argument for av_match_name,
> >> which takes const char* ...
> >> And mime_type was added as const char* to AVInputFormat, so this is
> >> even inconsistent.
> >>
> >> So should this be changed to const char*?
> >
> > i would tend toward a yes.
> > Though in theory this could lead to software that builds with libav
> > and doesnt with ffmpeg
> > for example if someone assigns a array to pd.mime_type and then
> > writes into that by using pd.mime_type
> 
> Not only in theory, because if one tries to assign a uint8_t* to a const 
> char* variable or vice versa, compilation will fail with e.g.:
> error: invalid conversion from ‘uint8_t* {aka unsigned char*}’ to ‘const 
> char*’ [-fpermissive]
> 
> > its really more a question what the community prefers here
> > 100% compatibility or 99% and the more sane type
> 
> Thus I'd say that in order to retain compatibility the 0003 patch, 
> changing AVProbeData.mime_type from uint8_t* to const char*, should only 
> be applied, if this change is also applied in Libav, even though I think 
> it is wrong the way it is now.
> 
> Best regards,
> Andreas
> 
> 0: https://lists.libav.org/pipermail/libav-devel/2014-July/061038.html