diff mbox series

[FFmpeg-devel,1/3] avformat/matroskadec: Add a workaround for missing WavPack extradata

Message ID 20200326040539.24719-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/3] avformat/matroskadec: Add a workaround for missing WavPack extradata | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 26, 2020, 4:05 a.m. UTC 
mkvmerge versions 6.2 to 40.0 had a bug that made it not propagate the
WavPack extradata (containing the WavPack version) during remuxing from
a Matroska file; currently our demuxer would treat every WavPack block
encountered as invalid data (unless the WavPack stream is to be
discarded (i.e. the streams discard is >= AVDISCARD_ALL)) and try to
resync to the next level 1 element.

Luckily, the WavPack version is currently not really important; so we
fix this problem by assuming a version. David Bryant, the creator of
WavPack, recommended using version 0x410 (the most recent version) for
this. And this is what this commit does.

A FATE-test for this has been added.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
James has already uploaded the sample. Thanks for this.

 libavformat/matroskadec.c                            | 11 ++++++++++-
 tests/fate/matroska.mak                              |  5 +++++
 tests/ref/fate/matroska-wavpack-missing-codecprivate |  9 +++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 tests/ref/fate/matroska-wavpack-missing-codecprivate

Comments

Andreas Rheinhardt April 1, 2020, 7:53 a.m. UTC | #1 
Andreas Rheinhardt:
> mkvmerge versions 6.2 to 40.0 had a bug that made it not propagate the
> WavPack extradata (containing the WavPack version) during remuxing from
> a Matroska file; currently our demuxer would treat every WavPack block
> encountered as invalid data (unless the WavPack stream is to be
> discarded (i.e. the streams discard is >= AVDISCARD_ALL)) and try to
> resync to the next level 1 element.
> 
> Luckily, the WavPack version is currently not really important; so we
> fix this problem by assuming a version. David Bryant, the creator of
> WavPack, recommended using version 0x410 (the most recent version) for
> this. And this is what this commit does.
> 
> A FATE-test for this has been added.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> James has already uploaded the sample. Thanks for this.
> 
>  libavformat/matroskadec.c                            | 11 ++++++++++-
>  tests/fate/matroska.mak                              |  5 +++++
>  tests/ref/fate/matroska-wavpack-missing-codecprivate |  9 +++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/fate/matroska-wavpack-missing-codecprivate
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 4d7fdab99f..5b55606b98 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2613,6 +2613,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
>              ret = matroska_parse_flac(s, track, &extradata_offset);
>              if (ret < 0)
>                  return ret;
> +        } else if (codec_id == AV_CODEC_ID_WAVPACK && track->codec_priv.size < 2) {
> +            av_log(matroska->ctx, AV_LOG_INFO, "Assuming WavPack version 4.10 "
> +                   "in absence of valid CodecPrivate.\n");
> +            extradata_size = 2;
> +            extradata = av_mallocz(2 + AV_INPUT_BUFFER_PADDING_SIZE);
> +            if (!extradata)
> +                return AVERROR(ENOMEM);
> +            AV_WL16(extradata, 0x410);
>          } else if (codec_id == AV_CODEC_ID_PRORES && track->codec_priv.size == 4) {
>              fourcc = AV_RL32(track->codec_priv.data);
>          } else if (codec_id == AV_CODEC_ID_VP9 && track->codec_priv.size) {
> @@ -3165,9 +3173,10 @@ static int matroska_parse_wavpack(MatroskaTrack *track, uint8_t *src,
>      uint16_t ver;
>      int ret, offset = 0;
>  
> -    if (srclen < 12 || track->stream->codecpar->extradata_size < 2)
> +    if (srclen < 12)
>          return AVERROR_INVALIDDATA;
>  
> +    av_assert1(track->stream->codecpar->extradata_size >= 2);
>      ver = AV_RL16(track->stream->codecpar->extradata);
>  
>      samples = AV_RL32(src);
> diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
> index b9ed7322fd..93b5bff89a 100644
> --- a/tests/fate/matroska.mak
> +++ b/tests/fate/matroska.mak
> @@ -17,6 +17,11 @@ fate-matroska-remux: REF = 49a60ef91cf7302ab7276f9373f8a429
>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) += fate-matroska-xiph-lacing
>  fate-matroska-xiph-lacing: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy
>  
> +# This tests that the Matroska demuxer correctly demuxes WavPack
> +# without CodecPrivate; it also tests zlib compressed WavPack.
> +FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-wavpack-missing-codecprivate
> +fate-matroska-wavpack-missing-codecprivate: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/wavpack_missing_codecprivate.mka -c copy
> +
>  # This tests that the matroska demuxer supports decompressing
>  # zlib compressed tracks (both the CodecPrivate as well as the actual frames).
>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-zlib-decompression
> diff --git a/tests/ref/fate/matroska-wavpack-missing-codecprivate b/tests/ref/fate/matroska-wavpack-missing-codecprivate
> new file mode 100644
> index 0000000000..4645a86ff6
> --- /dev/null
> +++ b/tests/ref/fate/matroska-wavpack-missing-codecprivate
> @@ -0,0 +1,9 @@
> +#extradata 0:        2, 0x00240014
> +#tb 0: 11337/500000000
> +#media_type 0: audio
> +#codec_id 0: wavpack
> +#sample_rate 0: 44100
> +#channel_layout 0: 3
> +#channel_layout_name 0: stereo
> +0,          0,          0,    22051,    14778, 0x02819286
> +0,      22051,      22051,    22052,    14756, 0x21976243
> 
Any comments on this patchset? If not, I'd like to apply it tomorrow.

- Andreas
David Bryant April 1, 2020, 7:17 p.m. UTC | #2 
On 4/1/20 12:53 AM, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
>> mkvmerge versions 6.2 to 40.0 had a bug that made it not propagate the
>> WavPack extradata (containing the WavPack version) during remuxing from
>> a Matroska file; currently our demuxer would treat every WavPack block
>> encountered as invalid data (unless the WavPack stream is to be
>> discarded (i.e. the streams discard is >= AVDISCARD_ALL)) and try to
>> resync to the next level 1 element.
>>
>> Luckily, the WavPack version is currently not really important; so we
>> fix this problem by assuming a version. David Bryant, the creator of
>> WavPack, recommended using version 0x410 (the most recent version) for
>> this. And this is what this commit does.
>>
>> A FATE-test for this has been added.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> James has already uploaded the sample. Thanks for this.
>>
>>  libavformat/matroskadec.c                            | 11 ++++++++++-
>>  tests/fate/matroska.mak                              |  5 +++++
>>  tests/ref/fate/matroska-wavpack-missing-codecprivate |  9 +++++++++
>>  3 files changed, 24 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/ref/fate/matroska-wavpack-missing-codecprivate
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 4d7fdab99f..5b55606b98 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -2613,6 +2613,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
>>              ret = matroska_parse_flac(s, track, &extradata_offset);
>>              if (ret < 0)
>>                  return ret;
>> +        } else if (codec_id == AV_CODEC_ID_WAVPACK && track->codec_priv.size < 2) {
>> +            av_log(matroska->ctx, AV_LOG_INFO, "Assuming WavPack version 4.10 "
>> +                   "in absence of valid CodecPrivate.\n");
>> +            extradata_size = 2;
>> +            extradata = av_mallocz(2 + AV_INPUT_BUFFER_PADDING_SIZE);
>> +            if (!extradata)
>> +                return AVERROR(ENOMEM);
>> +            AV_WL16(extradata, 0x410);
>>          } else if (codec_id == AV_CODEC_ID_PRORES && track->codec_priv.size == 4) {
>>              fourcc = AV_RL32(track->codec_priv.data);
>>          } else if (codec_id == AV_CODEC_ID_VP9 && track->codec_priv.size) {
>> @@ -3165,9 +3173,10 @@ static int matroska_parse_wavpack(MatroskaTrack *track, uint8_t *src,
>>      uint16_t ver;
>>      int ret, offset = 0;
>>  
>> -    if (srclen < 12 || track->stream->codecpar->extradata_size < 2)
>> +    if (srclen < 12)
>>          return AVERROR_INVALIDDATA;
>>  
>> +    av_assert1(track->stream->codecpar->extradata_size >= 2);
>>      ver = AV_RL16(track->stream->codecpar->extradata);
>>  
>>      samples = AV_RL32(src);
>> diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
>> index b9ed7322fd..93b5bff89a 100644
>> --- a/tests/fate/matroska.mak
>> +++ b/tests/fate/matroska.mak
>> @@ -17,6 +17,11 @@ fate-matroska-remux: REF = 49a60ef91cf7302ab7276f9373f8a429
>>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) += fate-matroska-xiph-lacing
>>  fate-matroska-xiph-lacing: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy
>>  
>> +# This tests that the Matroska demuxer correctly demuxes WavPack
>> +# without CodecPrivate; it also tests zlib compressed WavPack.
>> +FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-wavpack-missing-codecprivate
>> +fate-matroska-wavpack-missing-codecprivate: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/wavpack_missing_codecprivate.mka -c copy
>> +
>>  # This tests that the matroska demuxer supports decompressing
>>  # zlib compressed tracks (both the CodecPrivate as well as the actual frames).
>>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-zlib-decompression
>> diff --git a/tests/ref/fate/matroska-wavpack-missing-codecprivate b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>> new file mode 100644
>> index 0000000000..4645a86ff6
>> --- /dev/null
>> +++ b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>> @@ -0,0 +1,9 @@
>> +#extradata 0:        2, 0x00240014
>> +#tb 0: 11337/500000000
>> +#media_type 0: audio
>> +#codec_id 0: wavpack
>> +#sample_rate 0: 44100
>> +#channel_layout 0: 3
>> +#channel_layout_name 0: stereo
>> +0,          0,          0,    22051,    14778, 0x02819286
>> +0,      22051,      22051,    22052,    14756, 0x21976243
>>
> Any comments on this patchset? If not, I'd like to apply it tomorrow.

This looks good to me. Thanks!

-David
Andreas Rheinhardt April 2, 2020, 5:41 a.m. UTC | #3 
David Bryant:
> On 4/1/20 12:53 AM, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> mkvmerge versions 6.2 to 40.0 had a bug that made it not propagate the
>>> WavPack extradata (containing the WavPack version) during remuxing from
>>> a Matroska file; currently our demuxer would treat every WavPack block
>>> encountered as invalid data (unless the WavPack stream is to be
>>> discarded (i.e. the streams discard is >= AVDISCARD_ALL)) and try to
>>> resync to the next level 1 element.
>>>
>>> Luckily, the WavPack version is currently not really important; so we
>>> fix this problem by assuming a version. David Bryant, the creator of
>>> WavPack, recommended using version 0x410 (the most recent version) for
>>> this. And this is what this commit does.
>>>
>>> A FATE-test for this has been added.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> James has already uploaded the sample. Thanks for this.
>>>
>>>  libavformat/matroskadec.c                            | 11 ++++++++++-
>>>  tests/fate/matroska.mak                              |  5 +++++
>>>  tests/ref/fate/matroska-wavpack-missing-codecprivate |  9 +++++++++
>>>  3 files changed, 24 insertions(+), 1 deletion(-)
>>>  create mode 100644 tests/ref/fate/matroska-wavpack-missing-codecprivate
>>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 4d7fdab99f..5b55606b98 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -2613,6 +2613,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
>>>              ret = matroska_parse_flac(s, track, &extradata_offset);
>>>              if (ret < 0)
>>>                  return ret;
>>> +        } else if (codec_id == AV_CODEC_ID_WAVPACK && track->codec_priv.size < 2) {
>>> +            av_log(matroska->ctx, AV_LOG_INFO, "Assuming WavPack version 4.10 "
>>> +                   "in absence of valid CodecPrivate.\n");
>>> +            extradata_size = 2;
>>> +            extradata = av_mallocz(2 + AV_INPUT_BUFFER_PADDING_SIZE);
>>> +            if (!extradata)
>>> +                return AVERROR(ENOMEM);
>>> +            AV_WL16(extradata, 0x410);
>>>          } else if (codec_id == AV_CODEC_ID_PRORES && track->codec_priv.size == 4) {
>>>              fourcc = AV_RL32(track->codec_priv.data);
>>>          } else if (codec_id == AV_CODEC_ID_VP9 && track->codec_priv.size) {
>>> @@ -3165,9 +3173,10 @@ static int matroska_parse_wavpack(MatroskaTrack *track, uint8_t *src,
>>>      uint16_t ver;
>>>      int ret, offset = 0;
>>>  
>>> -    if (srclen < 12 || track->stream->codecpar->extradata_size < 2)
>>> +    if (srclen < 12)
>>>          return AVERROR_INVALIDDATA;
>>>  
>>> +    av_assert1(track->stream->codecpar->extradata_size >= 2);
>>>      ver = AV_RL16(track->stream->codecpar->extradata);
>>>  
>>>      samples = AV_RL32(src);
>>> diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
>>> index b9ed7322fd..93b5bff89a 100644
>>> --- a/tests/fate/matroska.mak
>>> +++ b/tests/fate/matroska.mak
>>> @@ -17,6 +17,11 @@ fate-matroska-remux: REF = 49a60ef91cf7302ab7276f9373f8a429
>>>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) += fate-matroska-xiph-lacing
>>>  fate-matroska-xiph-lacing: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy
>>>  
>>> +# This tests that the Matroska demuxer correctly demuxes WavPack
>>> +# without CodecPrivate; it also tests zlib compressed WavPack.
>>> +FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-wavpack-missing-codecprivate
>>> +fate-matroska-wavpack-missing-codecprivate: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/wavpack_missing_codecprivate.mka -c copy
>>> +
>>>  # This tests that the matroska demuxer supports decompressing
>>>  # zlib compressed tracks (both the CodecPrivate as well as the actual frames).
>>>  FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-zlib-decompression
>>> diff --git a/tests/ref/fate/matroska-wavpack-missing-codecprivate b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>>> new file mode 100644
>>> index 0000000000..4645a86ff6
>>> --- /dev/null
>>> +++ b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>>> @@ -0,0 +1,9 @@
>>> +#extradata 0:        2, 0x00240014
>>> +#tb 0: 11337/500000000
>>> +#media_type 0: audio
>>> +#codec_id 0: wavpack
>>> +#sample_rate 0: 44100
>>> +#channel_layout 0: 3
>>> +#channel_layout_name 0: stereo
>>> +0,          0,          0,    22051,    14778, 0x02819286
>>> +0,      22051,      22051,    22052,    14756, 0x21976243
>>>
>> Any comments on this patchset? If not, I'd like to apply it tomorrow.
> 
> This looks good to me. Thanks!
> 
> -David
> 
Applied the set, thanks.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 4d7fdab99f..5b55606b98 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2613,6 +2613,14 @@  static int matroska_parse_tracks(AVFormatContext *s)
             ret = matroska_parse_flac(s, track, &extradata_offset);
             if (ret < 0)
                 return ret;
+        } else if (codec_id == AV_CODEC_ID_WAVPACK && track->codec_priv.size < 2) {
+            av_log(matroska->ctx, AV_LOG_INFO, "Assuming WavPack version 4.10 "
+                   "in absence of valid CodecPrivate.\n");
+            extradata_size = 2;
+            extradata = av_mallocz(2 + AV_INPUT_BUFFER_PADDING_SIZE);
+            if (!extradata)
+                return AVERROR(ENOMEM);
+            AV_WL16(extradata, 0x410);
         } else if (codec_id == AV_CODEC_ID_PRORES && track->codec_priv.size == 4) {
             fourcc = AV_RL32(track->codec_priv.data);
         } else if (codec_id == AV_CODEC_ID_VP9 && track->codec_priv.size) {
@@ -3165,9 +3173,10 @@  static int matroska_parse_wavpack(MatroskaTrack *track, uint8_t *src,
     uint16_t ver;
     int ret, offset = 0;
 
-    if (srclen < 12 || track->stream->codecpar->extradata_size < 2)
+    if (srclen < 12)
         return AVERROR_INVALIDDATA;
 
+    av_assert1(track->stream->codecpar->extradata_size >= 2);
     ver = AV_RL16(track->stream->codecpar->extradata);
 
     samples = AV_RL32(src);
diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
index b9ed7322fd..93b5bff89a 100644
--- a/tests/fate/matroska.mak
+++ b/tests/fate/matroska.mak
@@ -17,6 +17,11 @@  fate-matroska-remux: REF = 49a60ef91cf7302ab7276f9373f8a429
 FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) += fate-matroska-xiph-lacing
 fate-matroska-xiph-lacing: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy
 
+# This tests that the Matroska demuxer correctly demuxes WavPack
+# without CodecPrivate; it also tests zlib compressed WavPack.
+FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-wavpack-missing-codecprivate
+fate-matroska-wavpack-missing-codecprivate: CMD = framecrc -i $(TARGET_SAMPLES)/mkv/wavpack_missing_codecprivate.mka -c copy
+
 # This tests that the matroska demuxer supports decompressing
 # zlib compressed tracks (both the CodecPrivate as well as the actual frames).
 FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) += fate-matroska-zlib-decompression
diff --git a/tests/ref/fate/matroska-wavpack-missing-codecprivate b/tests/ref/fate/matroska-wavpack-missing-codecprivate
new file mode 100644
index 0000000000..4645a86ff6
--- /dev/null
+++ b/tests/ref/fate/matroska-wavpack-missing-codecprivate
@@ -0,0 +1,9 @@ 
+#extradata 0:        2, 0x00240014
+#tb 0: 11337/500000000
+#media_type 0: audio
+#codec_id 0: wavpack
+#sample_rate 0: 44100
+#channel_layout 0: 3
+#channel_layout_name 0: stereo
+0,          0,          0,    22051,    14778, 0x02819286
+0,      22051,      22051,    22052,    14756, 0x21976243