[FFmpeg-devel,07/10] lavf: document that AVStream::codecpar may be modified by lavf after avformat_write_header(). This is assumed not to break API because it's already true (see e.g. matroskaenc handling of new AAC extradata)

Submitted by Rodger Combs on March 14, 2018, 6:24 a.m.

Details

Message ID 20180314062445.89909-7-rodger.combs@gmail.com
State New
Headers show

Commit Message

Rodger Combs March 14, 2018, 6:24 a.m.
---
 libavformat/avformat.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

James Almer March 15, 2018, 10:46 p.m.
On 3/14/2018 3:24 AM, Rodger Combs wrote:
> This is assumed not to break API because it's already true (see e.g.
> matroskaenc handling of new AAC extradata)

That's an API violation from the muxer's part instead. The correct
approach is to fix the violation, not adapt the API around it.

> ---
>  libavformat/avformat.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 9e87d6cdac..5f0ebfc114 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1006,7 +1006,8 @@ typedef struct AVStream {
>       *
>       * - demuxing: filled by libavformat on stream creation or in
>       *             avformat_find_stream_info()
> -     * - muxing: filled by the caller before avformat_write_header()
> +     * - muxing: filled by the caller before avformat_write_header();
> +     * -         may be modified by libavformat afterwards

Personally I'd rather fix the current muxer-level violations and if
needed store any such changes in a separate struct in AVStreamInternal
instead when dealing with generic code (Namely patch 9/10 from this set).
But if others are fine with introducing this definition change then i
wont block it.

>       */
>      AVCodecParameters *codecpar;
>  
>
Michael Niedermayer March 15, 2018, 11:29 p.m.
On Wed, Mar 14, 2018 at 01:24:42AM -0500, Rodger Combs wrote:
> ---
>  libavformat/avformat.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 9e87d6cdac..5f0ebfc114 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1006,7 +1006,8 @@ typedef struct AVStream {
>       *
>       * - demuxing: filled by libavformat on stream creation or in
>       *             avformat_find_stream_info()
> -     * - muxing: filled by the caller before avformat_write_header()
> +     * - muxing: filled by the caller before avformat_write_header();
> +     * -         may be modified by libavformat afterwards
>       */

a generic "anything can change" is not really practical
a user app would have to check every field of codecpar and possibly insert
video scalers if resoluton changed, or all kinds of other filters and even
different encoders if codec_id was adjusted ...

[...]
Hendrik Leppkes March 16, 2018, 1:04 a.m.
On Fri, Mar 16, 2018 at 12:29 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Wed, Mar 14, 2018 at 01:24:42AM -0500, Rodger Combs wrote:
>> ---
>>  libavformat/avformat.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 9e87d6cdac..5f0ebfc114 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1006,7 +1006,8 @@ typedef struct AVStream {
>>       *
>>       * - demuxing: filled by libavformat on stream creation or in
>>       *             avformat_find_stream_info()
>> -     * - muxing: filled by the caller before avformat_write_header()
>> +     * - muxing: filled by the caller before avformat_write_header();
>> +     * -         may be modified by libavformat afterwards
>>       */
>
> a generic "anything can change" is not really practical
> a user app would have to check every field of codecpar and possibly insert
> video scalers if resoluton changed, or all kinds of other filters and even
> different encoders if codec_id was adjusted ...
>

I generally don't like this either. codecpar describes the codec as
given to the muxer by the user. The muxer has no business changing
things around in it.
If it can derive better values somehow, let it store it internally somewhere.

- Hendrik
Rodger Combs March 16, 2018, 6:38 a.m.
Could we just declare that lavf can update extradata (and nothing else) if it gets packets with new-extradata side-data? If not, I suppose we could either add something to AVStreamInternal, or do something internal in check_bitstream (and update movenc and matroskaenc, as both exhibit this behavior right now).

> On Mar 15, 2018, at 20:04, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> 
> On Fri, Mar 16, 2018 at 12:29 AM, Michael Niedermayer
> <michael@niedermayer.cc <mailto:michael@niedermayer.cc>> wrote:
>> On Wed, Mar 14, 2018 at 01:24:42AM -0500, Rodger Combs wrote:
>>> ---
>>> libavformat/avformat.h | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>> index 9e87d6cdac..5f0ebfc114 100644
>>> --- a/libavformat/avformat.h
>>> +++ b/libavformat/avformat.h
>>> @@ -1006,7 +1006,8 @@ typedef struct AVStream {
>>>      *
>>>      * - demuxing: filled by libavformat on stream creation or in
>>>      *             avformat_find_stream_info()
>>> -     * - muxing: filled by the caller before avformat_write_header()
>>> +     * - muxing: filled by the caller before avformat_write_header();
>>> +     * -         may be modified by libavformat afterwards
>>>      */
>> 
>> a generic "anything can change" is not really practical
>> a user app would have to check every field of codecpar and possibly insert
>> video scalers if resoluton changed, or all kinds of other filters and even
>> different encoders if codec_id was adjusted ...
>> 
> 
> I generally don't like this either. codecpar describes the codec as
> given to the muxer by the user. The muxer has no business changing
> things around in it.
> If it can derive better values somehow, let it store it internally somewhere.
> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>

Patch hide | download patch | download mbox

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 9e87d6cdac..5f0ebfc114 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1006,7 +1006,8 @@  typedef struct AVStream {
      *
      * - demuxing: filled by libavformat on stream creation or in
      *             avformat_find_stream_info()
-     * - muxing: filled by the caller before avformat_write_header()
+     * - muxing: filled by the caller before avformat_write_header();
+     * -         may be modified by libavformat afterwards
      */
     AVCodecParameters *codecpar;