mbox series

[FFmpeg-devel,0/2] avformat/mvdec: make audio stream conditional

Message ID 20211231195019.16191-1-jpstewart@personalprojects.net
Headers show
Series avformat/mvdec: make audio stream conditional | expand

Message

John-Paul Stewart Dec. 31, 2021, 7:50 p.m. UTC
Recent discussion on the list led me to realize that libavformat was
unconditionally creating an audio stream for all SGI movie format
(version 2) files, even when no audio is present in the file.  

A sample of a movie file with no audio can be found at
    http://www.personalprojects.net/ffmpeg/silent.movie

Unpatched ffmpeg will report an audio stream even though no audio is
present.  After the following patch no audio stream is reported.  

SGI movie files with audio are slightly affected by the fact that the
audio stream is now allocated after the video stream, changing the order
they are listed in the output of ffprobe or ffmpeg.  I don't think this
materially affects anything.  All existing FATE tests pass.

Incidentally, the silent.movie sample above is at 25fps and can also be
used by anyone who wants to double-check the earlier patch 3c9ffbd009
that reads and sets the framerate.  The sample file is only about 88 KB.

Comments

Andreas Rheinhardt Dec. 31, 2021, 10:19 p.m. UTC | #1
John-Paul Stewart:
> Recent discussion on the list led me to realize that libavformat was
> unconditionally creating an audio stream for all SGI movie format
> (version 2) files, even when no audio is present in the file.  
> 
> A sample of a movie file with no audio can be found at
>     http://www.personalprojects.net/ffmpeg/silent.movie
> 
> Unpatched ffmpeg will report an audio stream even though no audio is
> present.  After the following patch no audio stream is reported.  
> 
> SGI movie files with audio are slightly affected by the fact that the
> audio stream is now allocated after the video stream, changing the order
> they are listed in the output of ffprobe or ffmpeg.  I don't think this
> materially affects anything.  All existing FATE tests pass.
> 

If I am not mistaken, it actually changes it a bit more: The audio data
(if present) is stored before the video data in the file and
mv_read_packet returns the data in the order of the stream numbers. Now
that you have reversed the order in which the streams are created, there
will be seeks; in particular, the file needs to be seekable.
This can be fixed by inverting the order in which packets are read in
mv_read_packet and by also creating the video streams for the
non-version-two files before the audio streams.

> Incidentally, the silent.movie sample above is at 25fps and can also be
> used by anyone who wants to double-check the earlier patch 3c9ffbd009
> that reads and sets the framerate.  The sample file is only about 88 KB.
>
John-Paul Stewart Jan. 1, 2022, 12:17 a.m. UTC | #2
On 2021-12-31 17:19, Andreas Rheinhardt wrote:
> John-Paul Stewart:
>> Recent discussion on the list led me to realize that libavformat was
>> unconditionally creating an audio stream for all SGI movie format
>> (version 2) files, even when no audio is present in the file.  
>>
>> A sample of a movie file with no audio can be found at
>>     http://www.personalprojects.net/ffmpeg/silent.movie
>>
>> Unpatched ffmpeg will report an audio stream even though no audio is
>> present.  After the following patch no audio stream is reported.  
>>
>> SGI movie files with audio are slightly affected by the fact that the
>> audio stream is now allocated after the video stream, changing the order
>> they are listed in the output of ffprobe or ffmpeg.  I don't think this
>> materially affects anything.  All existing FATE tests pass.
>>
> 
> If I am not mistaken, it actually changes it a bit more: The audio data
> (if present) is stored before the video data in the file and
> mv_read_packet returns the data in the order of the stream numbers. Now

Thanks for that info.  I hadn't realized that mv_read_packet relied on
the order of the stream numbers.

> that you have reversed the order in which the streams are created, there
> will be seeks; in particular, the file needs to be seekable.
> This can be fixed by inverting the order in which packets are read in
> mv_read_packet and by also creating the video streams for the
> non-version-two files before the audio streams.

Actually, I think it can be simpler than that.  The framerate is the
only piece of metadata for the video stream that is stored before the
audio/silent flag.  Since that's read into a local variable anyway, I
can just hold onto that one value until after creating the audio stream.
Then create the video stream in the same order as the original code and
minimize the changes.

I'll get to work on a v2 of the patch.
Andreas Rheinhardt Jan. 1, 2022, 12:33 a.m. UTC | #3
John-Paul Stewart:
> On 2021-12-31 17:19, Andreas Rheinhardt wrote:
>> John-Paul Stewart:
>>> Recent discussion on the list led me to realize that libavformat was
>>> unconditionally creating an audio stream for all SGI movie format
>>> (version 2) files, even when no audio is present in the file.  
>>>
>>> A sample of a movie file with no audio can be found at
>>>     http://www.personalprojects.net/ffmpeg/silent.movie
>>>
>>> Unpatched ffmpeg will report an audio stream even though no audio is
>>> present.  After the following patch no audio stream is reported.  
>>>
>>> SGI movie files with audio are slightly affected by the fact that the
>>> audio stream is now allocated after the video stream, changing the order
>>> they are listed in the output of ffprobe or ffmpeg.  I don't think this
>>> materially affects anything.  All existing FATE tests pass.
>>>
>>
>> If I am not mistaken, it actually changes it a bit more: The audio data
>> (if present) is stored before the video data in the file and
>> mv_read_packet returns the data in the order of the stream numbers. Now
> 
> Thanks for that info.  I hadn't realized that mv_read_packet relied on
> the order of the stream numbers.
> 

It is actually documented (right before creating the audio stream):
"/* allocate audio track first to prevent unnecessary seeking
  * (audio packet always precede video packet for a given frame) */"

- Andreas
John-Paul Stewart Jan. 1, 2022, 12:41 a.m. UTC | #4
On 2021-12-31 19:33, Andreas Rheinhardt wrote:
> John-Paul Stewart:
>> On 2021-12-31 17:19, Andreas Rheinhardt wrote:
>>> John-Paul Stewart:
>>>> Recent discussion on the list led me to realize that libavformat was
>>>> unconditionally creating an audio stream for all SGI movie format
>>>> (version 2) files, even when no audio is present in the file.  
>>>>
>>>> A sample of a movie file with no audio can be found at
>>>>     http://www.personalprojects.net/ffmpeg/silent.movie
>>>>
>>>> Unpatched ffmpeg will report an audio stream even though no audio is
>>>> present.  After the following patch no audio stream is reported.  
>>>>
>>>> SGI movie files with audio are slightly affected by the fact that the
>>>> audio stream is now allocated after the video stream, changing the order
>>>> they are listed in the output of ffprobe or ffmpeg.  I don't think this
>>>> materially affects anything.  All existing FATE tests pass.
>>>>
>>>
>>> If I am not mistaken, it actually changes it a bit more: The audio data
>>> (if present) is stored before the video data in the file and
>>> mv_read_packet returns the data in the order of the stream numbers. Now
>>
>> Thanks for that info.  I hadn't realized that mv_read_packet relied on
>> the order of the stream numbers.
>>
> 
> It is actually documented (right before creating the audio stream):
> "/* allocate audio track first to prevent unnecessary seeking
>   * (audio packet always precede video packet for a given frame) */"

I saw that, but thought it referred only to seeking through the metadata
in the file header.  I didn't realize that the comment meant the seeks
would happen later in processing the actual data stream in another
function.  I didn't look at the code in enough detail to fully
understand the comment.