diff mbox

[FFmpeg-devel,1/3] lavf: identify MP2 as a distinct container from MP3

Message ID 20161221044814.87331-1-rodger.combs@gmail.com
State Superseded
Headers show

Commit Message

Rodger Combs Dec. 21, 2016, 4:48 a.m. UTC
This allows us to report the correct codec ID here
---
 libavformat/allformats.c |  2 +-
 libavformat/mp3dec.c     | 62 +++++++++++++++++++++++++++++++-----------------
 libavformat/version.h    |  2 +-
 3 files changed, 42 insertions(+), 24 deletions(-)

Comments

Carl Eugen Hoyos Dec. 21, 2016, 8:27 a.m. UTC | #1
2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
> This allows us to report the correct codec ID here

Just curious: What does this fix?

Carl Eugen
Michael Niedermayer Dec. 21, 2016, 2:46 p.m. UTC | #2
On Tue, Dec 20, 2016 at 10:48:12PM -0600, Rodger Combs wrote:
> This allows us to report the correct codec ID here
> ---
>  libavformat/allformats.c |  2 +-
>  libavformat/mp3dec.c     | 62 +++++++++++++++++++++++++++++++-----------------
>  libavformat/version.h    |  2 +-
>  3 files changed, 42 insertions(+), 24 deletions(-)

this breaks demuxing some files

for example this one:
Input #0, mp3, from 'VIZ010-01_128.mups':
  Metadata:
    title           : VIZ010-01_128
    artist          : Joshua Iz feat. Chez Damier
    album           : Sentimental Love (Remixes)
    TIT1            : VIZ010
    copyright       : © & (P) Vizual Records 2011. All rights reserved.
    encoded_by      : iTunes 10.4.1
    date            : 2011
  Duration: 00:02:00.01, start: 0.000000, bitrate: 267 kb/s
    Stream #0:0: Audio: mp3, 44100 Hz, stereo, s16p, 128 kb/s
    Stream #0:1: Video: mjpeg, yuvj444p(pc, bt470bg/unknown/unknown), 1800x1800 [SAR 100:100 DAR 1:1], 90k tbr, 90k tbn, 90k tbc
    Metadata:
      comment         : Other
    Stream #0:2: Video: mjpeg, yuvj444p(pc, bt470bg/unknown/unknown), 1800x1800 [SAR 1:1 DAR 1:1], 90k tbr, 90k tbn, 90k tbc
    Metadata:
      comment         : Other

after the patch:

VIZ010-01_128.mups: Invalid data found when processing input

[...]
=?ISO-8859-1?B?c2Vh?= Dec. 21, 2016, 3:27 p.m. UTC | #3
In transcoding project of LeiXiaoHua(demo of usage of filters), if I use media file with h264 video stream as input file, the function  avcodec_open2 will return -1.  Media file with video stream in other format, such as mpeg4, mjpeg return 0 (OK). Why ? Someone meets the same problem?
Rodger Combs Dec. 21, 2016, 6:51 p.m. UTC | #4
> On Dec 21, 2016, at 02:27, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> 2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
>> This allows us to report the correct codec ID here
> 
> Just curious: What does this fix?

Reporting in ffprobe, or when using lavf as a library. Some devices and decoders either refuse to decode MP2, or need to be told that the input is MP2 as opposed to MP3 ahead of time. This also means we'll write the correct ID when remuxing.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
compn Dec. 21, 2016, 7:50 p.m. UTC | #5
On Wed, 21 Dec 2016 12:51:18 -0600
Rodger Combs <rodger.combs@gmail.com> wrote:

> 
> > On Dec 21, 2016, at 02:27, Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > wrote:
> > 
> > 2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
> >> This allows us to report the correct codec ID here
> > 
> > Just curious: What does this fix?
> 
> Reporting in ffprobe, or when using lavf as a library. Some devices
> and decoders either refuse to decode MP2, or need to be told that the
> input is MP2 as opposed to MP3 ahead of time. This also means we'll
> write the correct ID when remuxing.

if mp2 is a problem, i would rather make it more difficult for the user
to encode mp2. by defaulting to mp3 and / or printing a warning message
about mp2. honestly i cant remember any good coming from someone using
mp2.

mp2 is dead, long live mp3.

also please share your mp2 sample, if you have not already done so, if
possible.

-compn
James Almer Dec. 21, 2016, 8:05 p.m. UTC | #6
On 12/21/2016 4:50 PM, compn wrote:
> On Wed, 21 Dec 2016 12:51:18 -0600
> Rodger Combs <rodger.combs@gmail.com> wrote:
> 
>>
>>> On Dec 21, 2016, at 02:27, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> wrote:
>>>
>>> 2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
>>>> This allows us to report the correct codec ID here
>>>
>>> Just curious: What does this fix?
>>
>> Reporting in ffprobe, or when using lavf as a library. Some devices
>> and decoders either refuse to decode MP2, or need to be told that the
>> input is MP2 as opposed to MP3 ahead of time. This also means we'll
>> write the correct ID when remuxing.
> 
> if mp2 is a problem, i would rather make it more difficult for the user
> to encode mp2. by defaulting to mp3 and / or printing a warning message
> about mp2. honestly i cant remember any good coming from someone using
> mp2.

We use it to encode bitexact audio for FATE tests :P
Rodger Combs Dec. 21, 2016, 8:23 p.m. UTC | #7
> On Dec 21, 2016, at 13:50, compn <tempn@mi.rr.com> wrote:
> 
> On Wed, 21 Dec 2016 12:51:18 -0600
> Rodger Combs <rodger.combs@gmail.com> wrote:
> 
>> 
>>> On Dec 21, 2016, at 02:27, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> wrote:
>>> 
>>> 2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
>>>> This allows us to report the correct codec ID here
>>> 
>>> Just curious: What does this fix?
>> 
>> Reporting in ffprobe, or when using lavf as a library. Some devices
>> and decoders either refuse to decode MP2, or need to be told that the
>> input is MP2 as opposed to MP3 ahead of time. This also means we'll
>> write the correct ID when remuxing.
> 
> if mp2 is a problem, i would rather make it more difficult for the user
> to encode mp2. by defaulting to mp3 and / or printing a warning message
> about mp2. honestly i cant remember any good coming from someone using
> mp2.
> 
> mp2 is dead, long live mp3.
> 
> also please share your mp2 sample, if you have not already done so, if
> possible.

Here's the sample provided by a user: http://www.ste.no/jens/sample.mp4 <http://www.ste.no/jens/sample.mp4>
They said they're using MP2 because of some audio desync issue they get when using VLC to stream-dump ("well there's your problem"), but I don't think the reasoning behind this particular sample is relevant to the legitimacy of the issue.

> 
> -compn
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George Dec. 21, 2016, 8:59 p.m. UTC | #8
Le primidi 1er nivôse, an CCXXV, compn a écrit :
> if mp2 is a problem, i would rather make it more difficult for the user
> to encode mp2. by defaulting to mp3 and / or printing a warning message
> about mp2. honestly i cant remember any good coming from someone using
> mp2.
> 
> mp2 is dead, long live mp3.

IIRC, Michael said that it was probably rather easy to get the MP2
encoder do output a MP3 bistream, although without using the MP3
features and having only MP2 quality. Not something I would be able to
do, though.

Regards,
Tobias Rapp Dec. 22, 2016, 8:03 a.m. UTC | #9
On 21.12.2016 20:50, compn wrote:
> On Wed, 21 Dec 2016 12:51:18 -0600
> Rodger Combs <rodger.combs@gmail.com> wrote:
>
>>
>>> On Dec 21, 2016, at 02:27, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> wrote:
>>>
>>> 2016-12-21 5:48 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>:
>>>> This allows us to report the correct codec ID here
>>>
>>> Just curious: What does this fix?
>>
>> Reporting in ffprobe, or when using lavf as a library. Some devices
>> and decoders either refuse to decode MP2, or need to be told that the
>> input is MP2 as opposed to MP3 ahead of time. This also means we'll
>> write the correct ID when remuxing.
>
> if mp2 is a problem, i would rather make it more difficult for the user
> to encode mp2. by defaulting to mp3 and / or printing a warning message
> about mp2. honestly i cant remember any good coming from someone using
> mp2.
>
> mp2 is dead, long live mp3.

This might be true for home use scenarios but MP2 still has usage in 
some (legacy) broadcast environments. I agree that the FFmpeg-internal 
MP2 encoder seems optimized for speed rather than for quality, the 
TwoLame encoder is quite good, though.

Regards,
Tobias
diff mbox

Patch

diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index 6a79b75d78..6fd8aa609b 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -189,7 +189,7 @@  void av_register_all(void)
     REGISTER_DEMUXER (MM,               mm);
     REGISTER_MUXDEMUX(MMF,              mmf);
     REGISTER_MUXDEMUX(MOV,              mov);
-    REGISTER_MUXER   (MP2,              mp2);
+    REGISTER_MUXDEMUX(MP2,              mp2);
     REGISTER_MUXDEMUX(MP3,              mp3);
     REGISTER_MUXER   (MP4,              mp4);
     REGISTER_DEMUXER (MPC,              mpc);
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 099ca57d24..499aa45fd4 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -66,7 +66,7 @@  static int check(AVIOContext *pb, int64_t pos, uint32_t *header);
 
 /* mp3 read */
 
-static int mp3_read_probe(AVProbeData *p)
+static int mpa_read_probe(AVProbeData *p, int layer)
 {
     int max_frames, first_frames = 0;
     int whole_used = 0;
@@ -89,7 +89,7 @@  static int mp3_read_probe(AVProbeData *p)
 
             header = AV_RB32(buf2);
             ret = avpriv_mpegaudio_decode_header(&h, header);
-            if (ret != 0)
+            if (ret != 0 || h.layer != layer)
                 break;
             buf2 += h.frame_size;
         }
@@ -113,6 +113,12 @@  static int mp3_read_probe(AVProbeData *p)
 //mpegps_mp3_unrecognized_format.mpg has max_frames=3
 }
 
+#define READ_PROBE(l) \
+static int mp##l##_read_probe(AVProbeData *p) \
+{ \
+    return mpa_read_probe(p, l); \
+}
+
 static void read_xing_toc(AVFormatContext *s, int64_t filesize, int64_t duration)
 {
     int i;
@@ -341,7 +347,7 @@  static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base)
     return 0;
 }
 
-static int mp3_read_header(AVFormatContext *s)
+static int mpa_read_header(AVFormatContext *s, enum AVCodecID id)
 {
     MP3DecContext *mp3 = s->priv_data;
     AVStream *st;
@@ -354,7 +360,7 @@  static int mp3_read_header(AVFormatContext *s)
         return AVERROR(ENOMEM);
 
     st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
-    st->codecpar->codec_id = AV_CODEC_ID_MP3;
+    st->codecpar->codec_id = id;
     st->need_parsing = AVSTREAM_PARSE_FULL_RAW;
     st->start_time = 0;
 
@@ -419,6 +425,12 @@  static int mp3_read_header(AVFormatContext *s)
     return 0;
 }
 
+#define READ_HEADER(l) \
+static int mp##l##_read_header(AVFormatContext *s) \
+{ \
+    return mpa_read_header(s, AV_CODEC_ID_MP##l); \
+}
+
 #define MP3_PACKET_SIZE 1024
 
 static int mp3_read_packet(AVFormatContext *s, AVPacket *pkt)
@@ -579,23 +591,29 @@  static const AVOption options[] = {
     { NULL },
 };
 
-static const AVClass demuxer_class = {
-    .class_name = "mp3",
-    .item_name  = av_default_item_name,
-    .option     = options,
-    .version    = LIBAVUTIL_VERSION_INT,
-    .category   = AV_CLASS_CATEGORY_DEMUXER,
+#define DECLARE_LAYER(l, ext) \
+READ_PROBE(l) \
+READ_HEADER(l) \
+static const AVClass demuxer_class_##l = { \
+    .class_name = "mp" #l, \
+    .item_name  = av_default_item_name, \
+    .option     = options, \
+    .version    = LIBAVUTIL_VERSION_INT, \
+    .category   = AV_CLASS_CATEGORY_DEMUXER, \
+}; \
+\
+AVInputFormat ff_mp##l##_demuxer = { \
+    .name           = "mp" #l, \
+    .long_name      = NULL_IF_CONFIG_SMALL("MP" #l " (MPEG audio layer " #l ")"), \
+    .read_probe     = mp##l##_read_probe, \
+    .read_header    = mp##l##_read_header, \
+    .read_packet    = mp3_read_packet, \
+    .read_seek      = mp3_seek, \
+    .priv_data_size = sizeof(MP3DecContext), \
+    .flags          = AVFMT_GENERIC_INDEX, \
+    .extensions     = ext, /* XXX: use probe */ \
+    .priv_class     = &demuxer_class_##l, \
 };
 
-AVInputFormat ff_mp3_demuxer = {
-    .name           = "mp3",
-    .long_name      = NULL_IF_CONFIG_SMALL("MP2/3 (MPEG audio layer 2/3)"),
-    .read_probe     = mp3_read_probe,
-    .read_header    = mp3_read_header,
-    .read_packet    = mp3_read_packet,
-    .read_seek      = mp3_seek,
-    .priv_data_size = sizeof(MP3DecContext),
-    .flags          = AVFMT_GENERIC_INDEX,
-    .extensions     = "mp2,mp3,m2a,mpa", /* XXX: use probe */
-    .priv_class     = &demuxer_class,
-};
+DECLARE_LAYER(2, "mp2,m2a,mpa")
+DECLARE_LAYER(3, "mp3,mpa")
diff --git a/libavformat/version.h b/libavformat/version.h
index 65e6a4ccb7..21cc8a99c7 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  57
-#define LIBAVFORMAT_VERSION_MINOR  61
+#define LIBAVFORMAT_VERSION_MINOR  62
 #define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \