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

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

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

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.


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 1/2] avformat/mvdec: make audio stream conditional

[John-Paul Stewart]

Only allocate an audio stream if there is one in the data.  Silicon
Graphics movie format will contain default values (16 bit samples, 2
audio channels, 22050 Hz sample rate) even when no audio is present in
the file.  This confuses FFmpeg into thinking such an audio stream is
present with 0 samples in it.

There is a flag value in the format to indicate whether or not audio is
present.  This patch checks that and behaves accordingly.

Signed-off-by: John-Paul Stewart <jpstewart@personalprojects.net>
---
 libavformat/mvdec.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/libavformat/mvdec.c b/libavformat/mvdec.c
index ea955d2b11..7493bcc762 100644
--- a/libavformat/mvdec.c
+++ b/libavformat/mvdec.c
@@ -45,6 +45,10 @@ typedef struct MvContext {
     int aformat;          ///< audio format
 } MvContext;
 
+/* these magic numbers are defined in moviefile.h on Silicon Grahpics IRIX */
+#define MOVIE_SOUND  1
+#define MOVIE_SILENT 2
+
 #define AUDIO_FORMAT_SIGNED 401
 
 static int mv_probe(const AVProbeData *p)
@@ -305,20 +309,27 @@ static int mv_read_header(AVFormatContext *avctx)
 
         avio_skip(pb, 10);
 
-        /* allocate audio track first to prevent unnecessary seeking
-         * (audio packet always precede video packet for a given frame) */
-        ast = avformat_new_stream(avctx, NULL);
-        if (!ast)
-            return AVERROR(ENOMEM);
-
         vst = avformat_new_stream(avctx, NULL);
         if (!vst)
             return AVERROR(ENOMEM);
         fps = av_d2q(av_int2double(avio_rb64(pb)), INT_MAX);
         avpriv_set_pts_info(vst, 64, fps.den, fps.num);
-        avio_skip(pb, 4);
         vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
         vst->avg_frame_rate = fps;
+
+        v = avio_rb16(pb);
+        if (v == MOVIE_SOUND) {
+            /* movie has sound, so allocate an audio stream */
+            ast = avformat_new_stream(avctx, NULL);
+            if (!ast)
+                return AVERROR(ENOMEM);
+
+            ast->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
+        } else if (v != MOVIE_SILENT)
+            return AVERROR_INVALIDDATA;
+
+        avio_skip(pb, 2);
+
         vst->duration = vst->nb_frames = avio_rb32(pb);
         v = avio_rb32(pb);
         switch (v) {
@@ -338,7 +349,7 @@ static int mv_read_header(AVFormatContext *avctx)
         vst->codecpar->height    = avio_rb32(pb);
         avio_skip(pb, 12);
 
-        ast->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
+        if (ast) {
         ast->nb_frames          = vst->nb_frames;
         ast->codecpar->sample_rate = avio_rb32(pb);
         if (ast->codecpar->sample_rate <= 0) {
@@ -373,6 +384,8 @@ static int mv_read_header(AVFormatContext *avctx)
             return AVERROR_INVALIDDATA;
 
         avio_skip(pb, 8);
+        } else
+            avio_skip(pb, 24); /* skip meaningless audio metadata */
 
         var_read_metadata(avctx, "title", 0x80);
         var_read_metadata(avctx, "comment", 0x100);
@@ -386,9 +399,11 @@ static int mv_read_header(AVFormatContext *avctx)
             if (avio_feof(pb))
                 return AVERROR_INVALIDDATA;
             avio_skip(pb, 8);
+            if (ast) {
             av_add_index_entry(ast, pos, timestamp, asize, 0, AVINDEX_KEYFRAME);
-            av_add_index_entry(vst, pos + asize, i, vsize, 0, AVINDEX_KEYFRAME);
             timestamp += asize / (ast->codecpar->channels * (uint64_t)bytes_per_sample);
+            }
+            av_add_index_entry(vst, pos + asize, i, vsize, 0, AVINDEX_KEYFRAME);
         }
     } else if (!version && avio_rb16(pb) == 3) {
         avio_skip(pb, 4);
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/mvdec: re-indent after last commit

[John-Paul Stewart]

Signed-off-by: John-Paul Stewart <jpstewart@personalprojects.net>
---
 libavformat/mvdec.c | 64 ++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/libavformat/mvdec.c b/libavformat/mvdec.c
index 7493bcc762..469ac32092 100644
--- a/libavformat/mvdec.c
+++ b/libavformat/mvdec.c
@@ -350,40 +350,40 @@ static int mv_read_header(AVFormatContext *avctx)
         avio_skip(pb, 12);
 
         if (ast) {
-        ast->nb_frames          = vst->nb_frames;
-        ast->codecpar->sample_rate = avio_rb32(pb);
-        if (ast->codecpar->sample_rate <= 0) {
-            av_log(avctx, AV_LOG_ERROR, "Invalid sample rate %d\n", ast->codecpar->sample_rate);
-            return AVERROR_INVALIDDATA;
-        }
-        avpriv_set_pts_info(ast, 33, 1, ast->codecpar->sample_rate);
-
-        bytes_per_sample = avio_rb32(pb);
-
-        v = avio_rb32(pb);
-        if (v == AUDIO_FORMAT_SIGNED) {
-            switch (bytes_per_sample) {
-            case 1:
-                ast->codecpar->codec_id = AV_CODEC_ID_PCM_S8;
-                break;
-            case 2:
-                ast->codecpar->codec_id = AV_CODEC_ID_PCM_S16BE;
-                break;
-            default:
-                avpriv_request_sample(avctx, "Audio sample size %i bytes", bytes_per_sample);
-                break;
+            ast->nb_frames = vst->nb_frames;
+            ast->codecpar->sample_rate = avio_rb32(pb);
+            if (ast->codecpar->sample_rate <= 0) {
+                av_log(avctx, AV_LOG_ERROR, "Invalid sample rate %d\n", ast->codecpar->sample_rate);
+                return AVERROR_INVALIDDATA;
+            }
+            avpriv_set_pts_info(ast, 33, 1, ast->codecpar->sample_rate);
+
+            bytes_per_sample = avio_rb32(pb);
+
+            v = avio_rb32(pb);
+            if (v == AUDIO_FORMAT_SIGNED) {
+                switch (bytes_per_sample) {
+                case 1:
+                    ast->codecpar->codec_id = AV_CODEC_ID_PCM_S8;
+                    break;
+                case 2:
+                    ast->codecpar->codec_id = AV_CODEC_ID_PCM_S16BE;
+                    break;
+                default:
+                    avpriv_request_sample(avctx, "Audio sample size %i bytes", bytes_per_sample);
+                    break;
+                }
+            } else {
+                avpriv_request_sample(avctx, "Audio compression (format %i)", v);
             }
-        } else {
-            avpriv_request_sample(avctx, "Audio compression (format %i)", v);
-        }
 
-        if (bytes_per_sample == 0)
-            return AVERROR_INVALIDDATA;
+            if (bytes_per_sample == 0)
+                return AVERROR_INVALIDDATA;
 
-        if (set_channels(avctx, ast, avio_rb32(pb)) < 0)
-            return AVERROR_INVALIDDATA;
+            if (set_channels(avctx, ast, avio_rb32(pb)) < 0)
+                return AVERROR_INVALIDDATA;
 
-        avio_skip(pb, 8);
+            avio_skip(pb, 8);
         } else
             avio_skip(pb, 24); /* skip meaningless audio metadata */
 
@@ -400,8 +400,8 @@ static int mv_read_header(AVFormatContext *avctx)
                 return AVERROR_INVALIDDATA;
             avio_skip(pb, 8);
             if (ast) {
-            av_add_index_entry(ast, pos, timestamp, asize, 0, AVINDEX_KEYFRAME);
-            timestamp += asize / (ast->codecpar->channels * (uint64_t)bytes_per_sample);
+                av_add_index_entry(ast, pos, timestamp, asize, 0, AVINDEX_KEYFRAME);
+                timestamp += asize / (ast->codecpar->channels * (uint64_t)bytes_per_sample);
             }
             av_add_index_entry(vst, pos + asize, i, vsize, 0, AVINDEX_KEYFRAME);
         }
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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.
> 
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

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

> 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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[John-Paul Stewart]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".