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

[Andreas Cadhalpun]

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].

>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-doc-document-the-addition-of-the-AVProbeData.mime_ty.patch
Type: text/x-diff
Size: 1619 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140912/2a43de6f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-lavf-format.c-use-AVPROBE_SCORE_MIME-instead-of-AVPR.patch
Type: text/x-diff
Size: 1013 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140912/2a43de6f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavf-avformat.h-use-const-char-instead-of-uint8_t-fo.patch
Type: text/x-diff
Size: 1132 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140912/2a43de6f/attachment-0002.bin>