[FFmpeg-devel] [FFmpeg-cvslog] doc: document the addition of the AVProbeData.mime_type field and it' s implications

Sun Sep 14 00:22:02 CEST 2014

On 13.09.2014 23:54, Clément Bœsch wrote:
> On Sat, Sep 13, 2014 at 11:38:56PM +0200, Andreas Cadhalpun wrote:
>> On 13.09.2014 15:25, Michael Niedermayer wrote:
>>> On Sat, Sep 13, 2014 at 08:24:39AM +0200, Clément Bœsch wrote:
>>>> On Sat, Sep 13, 2014 at 12:53:21AM +0200, Andreas Cadhalpun wrote:
>>>>> ffmpeg | branch: master | Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com> | Fri Sep 12 18:18:42 2014 +0200| [d5e802609a0046441798cdbd137c96e4aa912390] | committer: Michael Niedermayer
>>>>>
>>>>> doc: document the addition of the AVProbeData.mime_type field and it's implications
>>>>>
>>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>>>>> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
>>>>>
>>>>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=d5e802609a0046441798cdbd137c96e4aa912390
>>>>> ---
>>>>>
>>>>>   RELEASE_NOTES  |    3 +++
>>>>>   doc/APIchanges |    3 +++
>>>>>   2 files changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/RELEASE_NOTES b/RELEASE_NOTES
>>>>> index 113cc5e..14513a7 100644
>>>>> --- a/RELEASE_NOTES
>>>>> +++ b/RELEASE_NOTES
>>>>> @@ -54,6 +54,9 @@
>>>>>    │ ⚠  Behaviour changes       │
>>>>>    └────────────────────────────┘
>>>>>
>>>>> +  • IMPORTANT: The new field mime_type was added to AVProbeData.
>>>>> +    To avoid crashes, make sure to always initialize AVProbeData, e.g. use
>>>>> +    'AVProbeData pd = { 0 };' instead of 'AVProbeData pd;'.
>>>>
>>>> I don't think we should mix API and UI in this file. The second sentence
>>>> can be moved to doc/APIchanges, that's what this file is used for. We
>>>> mention doc/APIchanges in that RELEASE_NOTES: "Please refer to the
>>>> doc/APIChanges file for more information."
>>
>
>> Indeed, that is a good idea. But still the other sentence doesn't really fit
>> into the 'Behaviour changes' section, so I moved it to the 'API Information'
>> section, where it fits much better and is directly followed by the pointer
>> to doc/APIChanges.
>
> I disagree, there is no reason to make an exception for that.

It would be good to mention this explicitly in the RELEASE_NOTES, 
because otherwise it just says the API didn't change and then people 
will be surprised that their programs crash.

>> In the attached patch I also changed the text at the beginning to only claim
>> that the API is mostly compatible and minimal source changes might be
>> needed.
>>
>> Best regards,
>> Andreas
>
>>  From 8170dc6151199bda655b3726f4cd129abe7781e1 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> Date: Sat, 13 Sep 2014 23:34:09 +0200
>> Subject: [PATCH] doc: don't mix API and UI changes in the 'Behaviour changes'
>>   section of the RELEASE_NOTES
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> ---
>>   RELEASE_NOTES  | 10 +++++-----
>>   doc/APIchanges |  2 ++
>>   2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/RELEASE_NOTES b/RELEASE_NOTES
>> index 14513a7..6c26aff 100644
>> --- a/RELEASE_NOTES
>> +++ b/RELEASE_NOTES
>> @@ -3,8 +3,8 @@
>>    ???????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
>>
>
> You have utf-8 troubles, the diff won't apply. Try git-send-email maybe.

Have you tried saving the attachement? It seems the last one did work...

>>      The FFmpeg Project proudly presents FFmpeg <next> "FIXME", ...
>> -   FFmpeg 2.4 is API-, but not ABI-compatible with the previous major release.
>> -   This means that the code using our libraries needs to be rebuilt, but no
>> +   FFmpeg 2.4 is mostly API-, but not ABI-compatible with the previous major release.
>> +   This means that the code using our libraries needs to be rebuilt, but only minimal
>>      source changes should be required.
>
> Unrelated

In a way it's related, since this is about the AVProbeData.mime_type API 
change, and it is confusing to state that the API didn't change, when it 
did.
But I can send a separate patch for that, if that's preferred.

>>
>>      ??????????????????????????????????????????????????????????????????????????????????????????
>> @@ -22,6 +22,9 @@
>>          ??? libswresample  xx.yy.1zz
>>          ??? libpostproc    xx.yy.1zz
>
>>
>> +     IMPORTANT: The new field mime_type was added to AVProbeData, which can
>> +     cause crashes, if it is not initialized.
>> +
>
> No, please just drop it.

Why don't you want to mention this here?

>>        Please refer to the doc/APIChanges file for more information.
>>
>>    ??????????????????????????????????????????????????????????????????????????????????????????
>> @@ -54,9 +57,6 @@
>>    ??? ???  Behaviour changes       ???
>>    ??????????????????????????????????????????????????????????????????????????????????????????
>>
>> -  ??? IMPORTANT: The new field mime_type was added to AVProbeData.
>> -    To avoid crashes, make sure to always initialize AVProbeData, e.g. use
>> -    'AVProbeData pd = { 0 };' instead of 'AVProbeData pd;'.
>>     ??? dctdnoiz filter now uses a block size of 8x8 instead of 16x16 by default
>>     ??? -vismv option is deprecated in favor of the codecview filter
>>     ??? libmodplug is now detected through pkg-config
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 90048a5..e3d402d 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -97,6 +97,8 @@ API changes, most recent first:
>>
>>   2014-07-29 - 80a3a66 / 3a19405 - lavf 56.01.100 / 56.01.0 - avformat.h
>>     Add mime_type field to AVProbeData.
>> +  To avoid crashes, make sure to always initialize AVProbeData, e.g. use
>> +
>
> I suggest the following:
>    Add mime_type field to AVProbeData, which now MUST be initialized in
>    order to avoid uninitialized reads of the mime_type pointer, likely
>    leading to crashes.
>    Typically, this means you will do 'AVProbeData pd = { 0 };' instead of
>    'AVProbeData pd;'.

This wording is fine.

> Maybe we will want to make a clear cut in the doc/APIChanges file to
> separate the releases, so this change will not be missed.

Yes, that might help.

Best regards,
Andreas