diff mbox series

[FFmpeg-devel] avformat/mov: fix demuxing of eia-608

Message ID 20200614122150.21574-1-onemda@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avformat/mov: fix demuxing of eia-608
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol June 14, 2020, 12:21 p.m. UTC
Fixes #4616.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/mov.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Kieran Kunhya June 14, 2020, 12:34 p.m. UTC | #1
On Sun, 14 Jun 2020 at 13:22, Paul B Mahol <onemda@gmail.com> wrote:

> Fixes #4616.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>

LGTM
Carl Eugen Hoyos June 14, 2020, 2:06 p.m. UTC | #2
Am So., 14. Juni 2020 um 14:22 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> Fixes #4616.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/mov.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2fc27d2aec..6d83a8a4b3 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7854,6 +7854,27 @@ static int mov_change_extradata(MOVStreamContext *sc, AVPacket *pkt)
>      return 0;
>  }
>
> +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
> +{
> +    int new_size, ret;
> +
> +    if (size <= 8)
> +        return AVERROR_INVALIDDATA;
> +    new_size = ((size - 8) / 2) * 3;
> +    ret = av_new_packet(pkt, new_size);
> +    if (ret < 0)
> +        return ret;
> +
> +    avio_skip(pb, 8);
> +    for (int j = 0; j < new_size; j += 3) {
> +        pkt->data[j] = 0xFC;
> +        pkt->data[j+1] = avio_r8(pb);
> +        pkt->data[j+2] = avio_r8(pb);
> +    }
> +
> +    return 0;
> +}
> +
>  static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>      MOVContext *mov = s->priv_data;
> @@ -7898,6 +7919,9 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>              goto retry;
>          }
>
> +        if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 && sample->size > 8)
> +            ret = get_eia608_packet(sc->pb, pkt, sample->size);
> +        else
>          ret = av_get_packet(sc->pb, pkt, sample->size);

I have to repeat the question that I raised in the ticket:
Doesn't this patch break remuxing of the subtitle stream?

Thank you for looking into this issue!

Carl Eugen
Paul B Mahol June 14, 2020, 3:07 p.m. UTC | #3
On 6/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am So., 14. Juni 2020 um 14:22 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>>
>> Fixes #4616.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/mov.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 2fc27d2aec..6d83a8a4b3 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -7854,6 +7854,27 @@ static int mov_change_extradata(MOVStreamContext
>> *sc, AVPacket *pkt)
>>      return 0;
>>  }
>>
>> +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
>> +{
>> +    int new_size, ret;
>> +
>> +    if (size <= 8)
>> +        return AVERROR_INVALIDDATA;
>> +    new_size = ((size - 8) / 2) * 3;
>> +    ret = av_new_packet(pkt, new_size);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    avio_skip(pb, 8);
>> +    for (int j = 0; j < new_size; j += 3) {
>> +        pkt->data[j] = 0xFC;
>> +        pkt->data[j+1] = avio_r8(pb);
>> +        pkt->data[j+2] = avio_r8(pb);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>>  {
>>      MOVContext *mov = s->priv_data;
>> @@ -7898,6 +7919,9 @@ static int mov_read_packet(AVFormatContext *s,
>> AVPacket *pkt)
>>              goto retry;
>>          }
>>
>> +        if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 && sample->size
>> > 8)
>> +            ret = get_eia608_packet(sc->pb, pkt, sample->size);
>> +        else
>>          ret = av_get_packet(sc->pb, pkt, sample->size);
>
> I have to repeat the question that I raised in the ticket:
> Doesn't this patch break remuxing of the subtitle stream?
>
> Thank you for looking into this issue!

Do you have files that previously worked when being muxed by ffmpeg into mov?
Thats is only scenario that no longer works, old invalid ffmpeg
generated mov files.
I guess similar patch should be done for mov muxer. If not already.

>
> Carl Eugen
> _______________________________________________
> 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".
Carl Eugen Hoyos June 14, 2020, 3:35 p.m. UTC | #4
Am So., 14. Juni 2020 um 17:08 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> On 6/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > Am So., 14. Juni 2020 um 14:22 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
> >>
> >> Fixes #4616.
> >>
> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> ---
> >>  libavformat/mov.c | 24 ++++++++++++++++++++++++
> >>  1 file changed, 24 insertions(+)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index 2fc27d2aec..6d83a8a4b3 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -7854,6 +7854,27 @@ static int mov_change_extradata(MOVStreamContext
> >> *sc, AVPacket *pkt)
> >>      return 0;
> >>  }
> >>
> >> +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
> >> +{
> >> +    int new_size, ret;
> >> +
> >> +    if (size <= 8)
> >> +        return AVERROR_INVALIDDATA;
> >> +    new_size = ((size - 8) / 2) * 3;
> >> +    ret = av_new_packet(pkt, new_size);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    avio_skip(pb, 8);
> >> +    for (int j = 0; j < new_size; j += 3) {
> >> +        pkt->data[j] = 0xFC;
> >> +        pkt->data[j+1] = avio_r8(pb);
> >> +        pkt->data[j+2] = avio_r8(pb);
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
> >>  {
> >>      MOVContext *mov = s->priv_data;
> >> @@ -7898,6 +7919,9 @@ static int mov_read_packet(AVFormatContext *s,
> >> AVPacket *pkt)
> >>              goto retry;
> >>          }
> >>
> >> +        if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 && sample->size
> >> > 8)
> >> +            ret = get_eia608_packet(sc->pb, pkt, sample->size);
> >> +        else
> >>          ret = av_get_packet(sc->pb, pkt, sample->size);
> >
> > I have to repeat the question that I raised in the ticket:
> > Doesn't this patch break remuxing of the subtitle stream?
> >
> > Thank you for looking into this issue!
>
> Do you have files that previously worked when being muxed by ffmpeg into mov?
> Thats is only scenario that no longer works, old invalid ffmpeg
> generated mov files.
> I guess similar patch should be done for mov muxer. If not already.

(hard to parse)

That was my question:
Once remuxing is fixed on the muxer side, will your patch still allow
remuxing at all?

Carl Eugen
Paul B Mahol June 14, 2020, 3:56 p.m. UTC | #5
On 6/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am So., 14. Juni 2020 um 17:08 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>>
>> On 6/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> > Am So., 14. Juni 2020 um 14:22 Uhr schrieb Paul B Mahol
>> > <onemda@gmail.com>:
>> >>
>> >> Fixes #4616.
>> >>
>> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >> ---
>> >>  libavformat/mov.c | 24 ++++++++++++++++++++++++
>> >>  1 file changed, 24 insertions(+)
>> >>
>> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> >> index 2fc27d2aec..6d83a8a4b3 100644
>> >> --- a/libavformat/mov.c
>> >> +++ b/libavformat/mov.c
>> >> @@ -7854,6 +7854,27 @@ static int mov_change_extradata(MOVStreamContext
>> >> *sc, AVPacket *pkt)
>> >>      return 0;
>> >>  }
>> >>
>> >> +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
>> >> +{
>> >> +    int new_size, ret;
>> >> +
>> >> +    if (size <= 8)
>> >> +        return AVERROR_INVALIDDATA;
>> >> +    new_size = ((size - 8) / 2) * 3;
>> >> +    ret = av_new_packet(pkt, new_size);
>> >> +    if (ret < 0)
>> >> +        return ret;
>> >> +
>> >> +    avio_skip(pb, 8);
>> >> +    for (int j = 0; j < new_size; j += 3) {
>> >> +        pkt->data[j] = 0xFC;
>> >> +        pkt->data[j+1] = avio_r8(pb);
>> >> +        pkt->data[j+2] = avio_r8(pb);
>> >> +    }
>> >> +
>> >> +    return 0;
>> >> +}
>> >> +
>> >>  static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>> >>  {
>> >>      MOVContext *mov = s->priv_data;
>> >> @@ -7898,6 +7919,9 @@ static int mov_read_packet(AVFormatContext *s,
>> >> AVPacket *pkt)
>> >>              goto retry;
>> >>          }
>> >>
>> >> +        if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 &&
>> >> sample->size
>> >> > 8)
>> >> +            ret = get_eia608_packet(sc->pb, pkt, sample->size);
>> >> +        else
>> >>          ret = av_get_packet(sc->pb, pkt, sample->size);
>> >
>> > I have to repeat the question that I raised in the ticket:
>> > Doesn't this patch break remuxing of the subtitle stream?
>> >
>> > Thank you for looking into this issue!
>>
>> Do you have files that previously worked when being muxed by ffmpeg into
>> mov?
>> Thats is only scenario that no longer works, old invalid ffmpeg
>> generated mov files.
>> I guess similar patch should be done for mov muxer. If not already.
>
> (hard to parse)
>
> That was my question:
> Once remuxing is fixed on the muxer side, will your patch still allow
> remuxing at all?

Yes, it will, after muxer is fixed too.
Why asking, do you think it will not?

Inside caption data packet is cdat atom, that this patch skips, also
only single text channel is supported so muxer will pick only 0xFC
captions with such first byte.

>
> Carl Eugen
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2fc27d2aec..6d83a8a4b3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7854,6 +7854,27 @@  static int mov_change_extradata(MOVStreamContext *sc, AVPacket *pkt)
     return 0;
 }
 
+static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
+{
+    int new_size, ret;
+
+    if (size <= 8)
+        return AVERROR_INVALIDDATA;
+    new_size = ((size - 8) / 2) * 3;
+    ret = av_new_packet(pkt, new_size);
+    if (ret < 0)
+        return ret;
+
+    avio_skip(pb, 8);
+    for (int j = 0; j < new_size; j += 3) {
+        pkt->data[j] = 0xFC;
+        pkt->data[j+1] = avio_r8(pb);
+        pkt->data[j+2] = avio_r8(pb);
+    }
+
+    return 0;
+}
+
 static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
     MOVContext *mov = s->priv_data;
@@ -7898,6 +7919,9 @@  static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
             goto retry;
         }
 
+        if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 && sample->size > 8)
+            ret = get_eia608_packet(sc->pb, pkt, sample->size);
+        else
         ret = av_get_packet(sc->pb, pkt, sample->size);
         if (ret < 0) {
             if (should_retry(sc->pb, ret)) {