[FFmpeg-devel] [PATCH] avformat/mov: (v4) fix get_eia608_packet

[Zhao Zhili]

> On Feb 27, 2025, at 12:35, Pavel Koshevoy <pkoshevoy at gmail.com> wrote:
> 
> On Wed, Feb 26, 2025 at 8:03 PM Zhao Zhili <
> quinkblack-at-foxmail.com at ffmpeg.org> wrote:
> 
>> 
>> 
>>> On Feb 24, 2025, at 00:47, Pavel Koshevoy <pkoshevoy at gmail.com> wrote:
>>> 
>>> On Fri, Feb 21, 2025 at 9:49 AM Pavel Koshevoy <pkoshevoy at gmail.com>
>> wrote:
>>> 
>>>> If there are no further constructive review comments today and tomorrow
>>>> then I will commit and push this change on Sunday (if I don't forget).
>>>> 
>>>> Pavel.
>>>> 
>>> 
>>> 
>>> Pushed in commit 5021764413a138f340ba46a9b145215b7535ea83
>> 
>> You should remove ‘(v4)’ before push.
>> 
>> 
> 
> That would have been good to know two weeks ago, but thank you.

I think it’s a common practice for maintainer to update commit message before push
if the commit message is inappropriate.

Maybe new CI workflow can make this easy to operate.

> 
> Pavel
> 
> 
> 
> 
>>> 
>>> Pavel.
>>> 
>>> 
>>> 
>>> 
>>>> 
>>>> On Thu, Feb 13, 2025 at 2:22 PM Pavel Koshevoy <pkoshevoy at gmail.com>
>>>> wrote:
>>>> 
>>>>> The problem is reproducible with "Test for Quicktime 608 CC file.mov"
>>>>> from https://samples.ffmpeg.org/MPEG2/subcc/
>>>>> 
>>>>> ffmpeg -i "Test for Quicktime 608 CC file.mov" -map 0 -c copy -y
>>>>> remuxed.mov
>>>>> 
>>>>> Prior to the fix QuickTime Player playback of remuxed.mov would
>>>>> render garbage text for "English CC" subtitles.
>>>>> ---
>>>>> libavformat/mov.c | 70 +++++++++++++++++++++++++++++++++++++++--------
>>>>> 1 file changed, 59 insertions(+), 11 deletions(-)
>>>>> 
>>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>>> index 85aef33b19..5a91ef5b8c 100644
>>>>> --- a/libavformat/mov.c
>>>>> +++ b/libavformat/mov.c
>>>>> @@ -10788,25 +10788,73 @@ static int mov_change_extradata(AVStream *st,
>>>>> AVPacket *pkt)
>>>>>    return 0;
>>>>> }
>>>>> 
>>>>> -static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
>>>>> +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int
>>>>> src_size)
>>>>> {
>>>>> -    int new_size, ret;
>>>>> +    /* We can't make assumptions about the structure of the payload,
>>>>> +       because it may include multiple cdat and cdt2 samples. */
>>>>> +    const uint32_t cdat = AV_RB32("cdat");
>>>>> +    const uint32_t cdt2 = AV_RB32("cdt2");
>>>>> +    int ret, out_size = 0;
>>>>> 
>>>>> -    if (size <= 8)
>>>>> +    /* a valid payload must have size, 4cc, and at least 1 byte pair:
>> */
>>>>> +    if (src_size < 10)
>>>>>        return AVERROR_INVALIDDATA;
>>>>> -    new_size = ((size - 8) / 2) * 3;
>>>>> -    ret = av_new_packet(pkt, new_size);
>>>>> +
>>>>> +    /* avoid an int overflow: */
>>>>> +    if ((src_size - 8) / 2 >= INT_MAX / 3)
>>>>> +        return AVERROR_INVALIDDATA;
>>>>> +
>>>>> +    ret = av_new_packet(pkt, ((src_size - 8) / 2) * 3);
>>>>>    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);
>>>>> +    /* parse and re-format the c608 payload in one pass. */
>>>>> +    while (src_size >= 10) {
>>>>> +        const uint32_t atom_size = avio_rb32(pb);
>>>>> +        const uint32_t atom_type = avio_rb32(pb);
>>>>> +        const uint32_t data_size = atom_size - 8;
>>>>> +        const uint8_t cc_field =
>>>>> +            atom_type == cdat ? 1 :
>>>>> +            atom_type == cdt2 ? 2 :
>>>>> +            0;
>>>>> +
>>>>> +        /* account for bytes consumed for atom size and type. */
>>>>> +        src_size -= 8;
>>>>> +
>>>>> +        /* make sure the data size stays within the buffer
>> boundaries. */
>>>>> +        if (data_size < 2 || data_size > src_size) {
>>>>> +            ret = AVERROR_INVALIDDATA;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        /* make sure the data size is consistent with N byte pairs. */
>>>>> +        if (data_size % 2 != 0) {
>>>>> +            ret = AVERROR_INVALIDDATA;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        if (!cc_field) {
>>>>> +            /* neither cdat or cdt2 ... skip it */
>>>>> +            avio_skip(pb, data_size);
>>>>> +            src_size -= data_size;
>>>>> +            continue;
>>>>> +        }
>>>>> +
>>>>> +        for (int32_t i = 0; i < data_size; i += 2) {
>>>>> +            pkt->data[out_size] = (0x1F << 3) | (1 << 2) | (cc_field -
>>>>> 1);
>>>>> +            pkt->data[out_size + 1] = avio_r8(pb);
>>>>> +            pkt->data[out_size + 2] = avio_r8(pb);
>>>>> +            out_size += 3;
>>>>> +            src_size -= 2;
>>>>> +        }
>>>>>    }
>>>>> 
>>>>> -    return 0;
>>>>> +    if (src_size > 0)
>>>>> +        /* skip any remaining unread portion of the input payload */
>>>>> +        avio_skip(pb, src_size);
>>>>> +
>>>>> +    av_shrink_packet(pkt, out_size);
>>>>> +    return ret;
>>>>> }
>>>>> 
>>>>> static int mov_finalize_packet(AVFormatContext *s, AVStream *st,
>>>>> AVIndexEntry *sample,
>>>>> --
>>>>> 2.43.0
>>>>> 
>>>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> 
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".