diff mbox

[FFmpeg-devel,18/21] libavformat/movenc: mov_write_audio_tag writes the proper number of channels, not the hardcoded 2

Message ID 1471943019-14136-19-git-send-email-erkki.seppala.ext@nokia.com
State Superseded
Headers show

Commit Message

erkki.seppala.ext@nokia.com Aug. 23, 2016, 9:03 a.m. UTC
From: Erkki Seppälä <erkki.seppala.ext@nokia.com>

Signed-off-by: Erkki Seppälä <erkki.seppala.ext@nokia.com>
Signed-off-by: OZOPlayer <OZOPL@nokia.com>
---
 libavformat/movenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yusuke Nakamura Aug. 23, 2016, 3:20 p.m. UTC | #1
2016-08-23 18:03 GMT+09:00 <erkki.seppala.ext@nokia.com>:

> From: Erkki Seppälä <erkki.seppala.ext@nokia.com>
>
> Signed-off-by: Erkki Seppälä <erkki.seppala.ext@nokia.com>
> Signed-off-by: OZOPlayer <OZOPL@nokia.com>
> ---
>  libavformat/movenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 94d978b..020d13d 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -1039,7 +1039,7 @@ static int mov_write_audio_tag(AVFormatContext *s,
> AVIOContext *pb, MOVMuxContex
>                  avio_wb16(pb, 16);
>              avio_wb16(pb, track->audio_vbr ? -2 : 0); /* compression ID */
>          } else { /* reserved for mp4/3gp */
> -            avio_wb16(pb, 2);
> +            avio_wb16(pb, track->par->channels);
>

No. the ChannelCount field is templated. It may be fixed to 2 by derived
specs or the specs of codec encapsulations. I mean the current
implemetation is wrong, and your implementation is also wrong. For
instance, ChannelCount of AC-3 in ISOBMFF is always hardcoded to 2 even if
5.1ch. This field is basically not useful for ISOBMFF and the actual
channels shall be referred to Codec specific info.

             avio_wb16(pb, 16);
>              avio_wb16(pb, 0);
>          }
> --
> 2.7.4
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
erkki.seppala.ext@nokia.com Aug. 24, 2016, 7:14 a.m. UTC | #2
On 08/23/2016 06:20 PM, Yusuke Nakamura wrote:
>> -            avio_wb16(pb, 2);
>> +            avio_wb16(pb, track->par->channels);
> No. the ChannelCount field is templated. It may be fixed to 2 by derived
> specs or the specs of codec encapsulations. I mean the current
> implemetation is wrong, and your implementation is also wrong. For
> instance, ChannelCount of AC-3 in ISOBMFF is always hardcoded to 2 even if
> 5.1ch. This field is basically not useful for ISOBMFF and the actual
> channels shall be referred to Codec specific info.

It seems the part of the standard didn't give the full view on the 
matter. This patch can be dropped.

Thanks for the review!
diff mbox

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 94d978b..020d13d 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1039,7 +1039,7 @@  static int mov_write_audio_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContex
                 avio_wb16(pb, 16);
             avio_wb16(pb, track->audio_vbr ? -2 : 0); /* compression ID */
         } else { /* reserved for mp4/3gp */
-            avio_wb16(pb, 2);
+            avio_wb16(pb, track->par->channels);
             avio_wb16(pb, 16);
             avio_wb16(pb, 0);
         }