diff mbox series

[FFmpeg-devel] avcodec/dv_profile: dv files with wrong dsf flag - detect via buf_size

Message ID 9017e9a3-8701-e787-7ec8-b576ed7accab@mark-plomer.de
State New
Headers show
Series [FFmpeg-devel] avcodec/dv_profile: dv files with wrong dsf flag - detect via buf_size
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Mark Plomer Jan. 16, 2021, 1:40 p.m. UTC
Trying to fix https://trac.ffmpeg.org/ticket/8333

Some of my old DV AVI files have the DSF-Flag of frames set to 0, 
although it is PAL (I think they were rendered with Ulead Media Studio 
Pro) ... this causes ffmpeg/VLC-player to produce/play corrupted video.

In other players/editors it works fine including:

- VirtualDub
- Windows Media Player
- AVCutty
- Ulead Media Studio Pro (very old)

I had a look at VirtualDub ... there the PAL/NTSC detection is based on 
the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211

As I don't know, how to access the AVI header info inside 
dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another workaround 
by checking the buf_size against the dv_profile.

It works fine now, but I don't know, if this is really the best solution ...
Subject: [PATCH] avcodec/dv_profile: dv files with wrong dsf flag - detect via
 buf_size

---
 libavcodec/dv_profile.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mark Plomer March 13, 2021, 9:02 a.m. UTC | #1
Hello,

is there anyone who maintains the "dvvideo" module, who can review this 
patch and give me some feedback?

Greets Mark

Am 16.01.21 um 14:40 schrieb Mark Plomer:
> Trying to fix https://trac.ffmpeg.org/ticket/8333
>
> Some of my old DV AVI files have the DSF-Flag of frames set to 0, 
> although it is PAL (I think they were rendered with Ulead Media Studio 
> Pro) ... this causes ffmpeg/VLC-player to produce/play corrupted video.
>
> In other players/editors it works fine including:
>
> - VirtualDub
> - Windows Media Player
> - AVCutty
> - Ulead Media Studio Pro (very old)
>
> I had a look at VirtualDub ... there the PAL/NTSC detection is based 
> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
>
> As I don't know, how to access the AVI header info inside 
> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another 
> workaround by checking the buf_size against the dv_profile.
>
> It works fine now, but I don't know, if this is really the best 
> solution ...
>
>
> _______________________________________________
> 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".
Michael Niedermayer March 13, 2021, 10:41 a.m. UTC | #2
On Sat, Jan 16, 2021 at 02:40:03PM +0100, Mark Plomer wrote:
> Trying to fix https://trac.ffmpeg.org/ticket/8333
> 
> Some of my old DV AVI files have the DSF-Flag of frames set to 0, although
> it is PAL (I think they were rendered with Ulead Media Studio Pro) ... this
> causes ffmpeg/VLC-player to produce/play corrupted video.
> 
> In other players/editors it works fine including:
> 
> - VirtualDub
> - Windows Media Player
> - AVCutty
> - Ulead Media Studio Pro (very old)
> 
> I had a look at VirtualDub ... there the PAL/NTSC detection is based on the
> frame rate from AVISTREAMINFO header (dwRate/dwScale) - see https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
> 
> As I don't know, how to access the AVI header info inside
> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another workaround by
> checking the buf_size against the dv_profile.
> 
> It works fine now, but I don't know, if this is really the best solution ...
> 

>  dv_profile.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 9676b11bedef27c1c2a359c7617ecc6d30044b16  0001-avcodec-dv_profile-dv-files-with-wrong-dsf-flag.patch
> From 9dd240e6700c28601014e79e55d3ddee5f6b13c7 Mon Sep 17 00:00:00 2001
> From: Mark Plomer <not-implemented@mark-plomer.de>
> Date: Sat, 16 Jan 2021 14:04:44 +0100
> Subject: [PATCH] avcodec/dv_profile: dv files with wrong dsf flag - detect via
>  buf_size
> 
> ---

The commit message should mention "Fixes Ticket8333" if that is a bugfix 
for that ticket

also maybe some of the other details you write in your mail could be moved
into the commit message

Thanks

[...]
Marton Balint March 13, 2021, 4:48 p.m. UTC | #3
On Sat, 16 Jan 2021, Mark Plomer wrote:

> Trying to fix https://trac.ffmpeg.org/ticket/8333
>
> Some of my old DV AVI files have the DSF-Flag of frames set to 0, although it 
> is PAL (I think they were rendered with Ulead Media Studio Pro) ... this 
> causes ffmpeg/VLC-player to produce/play corrupted video.
>
> In other players/editors it works fine including:
>
> - VirtualDub
> - Windows Media Player
> - AVCutty
> - Ulead Media Studio Pro (very old)
>
> I had a look at VirtualDub ... there the PAL/NTSC detection is based on the 
> frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
>
> As I don't know, how to access the AVI header info inside 
> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another workaround by 
> checking the buf_size against the dv_profile.
>
> It works fine now, but I don't know, if this is really the best solution ...

--- a/libavcodec/dv_profile.c
+++ b/libavcodec/dv_profile.c
@@ -281,6 +281,10 @@ const AVDVProfile* 
ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile
         && codec->coded_height==576)
          return &dv_profiles[1];

+    /* hack for trac issue #8333, dv files with wrong dsf flag - detect via buf_size */
+    if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == dv_profiles[1].frame_size)
+        return &dv_profiles[1];
+

If possible, then it is probably better to move this fallback to later in 
the code, right after the hack for trac issue #217, so previous hacks 
won't get broken...

Regards,
Marton
Mark Plomer March 16, 2021, 8:51 p.m. UTC | #4
Unfortunately it is not possible to move the hack to the bottom, because 
the "normal processing" (dv_profiles lookup loop) will catch it 
otherwise (with a wrong profile), because it does not check the frame_size:

     for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++)
         if (dsf == dv_profiles[i].dsf && stype == 
dv_profiles[i].video_stype)
             return &dv_profiles[i];

But as the new "if" is very specific and catches an "invalid" 
combination only (with the frame_size check), it should not break 
anything, I think:

     if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == 
dv_profiles[1].frame_size)

At least, I put the new hack after all other "pre-normal-processing" 
hacks ;-)

Regards
Mark

Am 13.03.21 um 17:48 schrieb Marton Balint:
>
>
> On Sat, 16 Jan 2021, Mark Plomer wrote:
>
>> Trying to fix https://trac.ffmpeg.org/ticket/8333
>>
>> Some of my old DV AVI files have the DSF-Flag of frames set to 0, 
>> although it is PAL (I think they were rendered with Ulead Media 
>> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play 
>> corrupted video.
>>
>> In other players/editors it works fine including:
>>
>> - VirtualDub
>> - Windows Media Player
>> - AVCutty
>> - Ulead Media Studio Pro (very old)
>>
>> I had a look at VirtualDub ... there the PAL/NTSC detection is based 
>> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
>> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
>>
>> As I don't know, how to access the AVI header info inside 
>> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another 
>> workaround by checking the buf_size against the dv_profile.
>>
>> It works fine now, but I don't know, if this is really the best 
>> solution ...
>
> --- a/libavcodec/dv_profile.c
> +++ b/libavcodec/dv_profile.c
> @@ -281,6 +281,10 @@ const AVDVProfile* 
> ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile
>         && codec->coded_height==576)
>          return &dv_profiles[1];
>
> +    /* hack for trac issue #8333, dv files with wrong dsf flag - 
> detect via buf_size */
> +    if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size 
> == dv_profiles[1].frame_size)
> +        return &dv_profiles[1];
> +
>
> If possible, then it is probably better to move this fallback to later 
> in the code, right after the hack for trac issue #217, so previous 
> hacks won't get broken...
>
> Regards,
> Marton
> _______________________________________________
> 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".
Mark Plomer March 16, 2021, 9:06 p.m. UTC | #5
Oh yes, of course. I updated the commit-message in the attached new patch.

Am 13.03.21 um 11:41 schrieb Michael Niedermayer:
>
> The commit message should mention "Fixes Ticket8333" if that is a bugfix
> for that ticket
>
> also maybe some of the other details you write in your mail could be moved
> into the commit message
>
> Thanks
>
> [...]
>
Marton Balint March 16, 2021, 10:09 p.m. UTC | #6
On Tue, 16 Mar 2021, Mark Plomer wrote:

> Unfortunately it is not possible to move the hack to the bottom, because 
> the "normal processing" (dv_profiles lookup loop) will catch it 
> otherwise (with a wrong profile), because it does not check the frame_size:

Then at least check the 50/60 flag as well in the VAUX source pack, e.g.:

pal   = !!(frame[80 * 5 + 48 + 3] & 0x20);

Regards,
Marton


>
>     for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++)
>         if (dsf == dv_profiles[i].dsf && stype == 
> dv_profiles[i].video_stype)
>             return &dv_profiles[i];
>
> But as the new "if" is very specific and catches an "invalid" 
> combination only (with the frame_size check), it should not break 
> anything, I think:
>
>     if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == 
> dv_profiles[1].frame_size)
>
> At least, I put the new hack after all other "pre-normal-processing" 
> hacks ;-)
>
> Regards
> Mark
>
> Am 13.03.21 um 17:48 schrieb Marton Balint:
>>
>>
>> On Sat, 16 Jan 2021, Mark Plomer wrote:
>>
>>> Trying to fix https://trac.ffmpeg.org/ticket/8333
>>>
>>> Some of my old DV AVI files have the DSF-Flag of frames set to 0, 
>>> although it is PAL (I think they were rendered with Ulead Media 
>>> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play 
>>> corrupted video.
>>>
>>> In other players/editors it works fine including:
>>>
>>> - VirtualDub
>>> - Windows Media Player
>>> - AVCutty
>>> - Ulead Media Studio Pro (very old)
>>>
>>> I had a look at VirtualDub ... there the PAL/NTSC detection is based 
>>> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
>>> 
> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
>>>
>>> As I don't know, how to access the AVI header info inside 
>>> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another 
>>> workaround by checking the buf_size against the dv_profile.
>>>
>>> It works fine now, but I don't know, if this is really the best 
>>> solution ...
>>
>> --- a/libavcodec/dv_profile.c
>> +++ b/libavcodec/dv_profile.c
>> @@ -281,6 +281,10 @@ const AVDVProfile* 
>> ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile
>>         && codec->coded_height==576)
>>          return &dv_profiles[1];
>>
>> +    /* hack for trac issue #8333, dv files with wrong dsf flag - 
>> detect via buf_size */
>> +    if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size 
>> == dv_profiles[1].frame_size)
>> +        return &dv_profiles[1];
>> +
>>
>> If possible, then it is probably better to move this fallback to later 
>> in the code, right after the hack for trac issue #217, so previous 
>> hacks won't get broken...
>>
>> Regards,
>> Marton
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
Marton Balint March 16, 2021, 10:12 p.m. UTC | #7
On Tue, 16 Mar 2021, Marton Balint wrote:

>
>
> On Tue, 16 Mar 2021, Mark Plomer wrote:
>
>> Unfortunately it is not possible to move the hack to the bottom, because 
>> the "normal processing" (dv_profiles lookup loop) will catch it 
>> otherwise (with a wrong profile), because it does not check the frame_size:
>
> Then at least check the 50/60 flag as well in the VAUX source pack, e.g.:
>
> pal   = !!(frame[80 * 5 + 48 + 3] & 0x20);

And as far as I see, the more specific hack checking for codec_tag and 
similar can be removed then, because your check covers that case as well.

Regards,
Marton

>
> Regards,
> Marton
>
>
>>
>>     for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++)
>>         if (dsf == dv_profiles[i].dsf && stype == 
>> dv_profiles[i].video_stype)
>>             return &dv_profiles[i];
>>
>> But as the new "if" is very specific and catches an "invalid" 
>> combination only (with the frame_size check), it should not break 
>> anything, I think:
>>
>>     if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == 
>> dv_profiles[1].frame_size)
>>
>> At least, I put the new hack after all other "pre-normal-processing" 
>> hacks ;-)
>>
>> Regards
>> Mark
>>
>> Am 13.03.21 um 17:48 schrieb Marton Balint:
>>>
>>>
>>> On Sat, 16 Jan 2021, Mark Plomer wrote:
>>>
>>>> Trying to fix https://trac.ffmpeg.org/ticket/8333
>>>>
>>>> Some of my old DV AVI files have the DSF-Flag of frames set to 0, 
>>>> although it is PAL (I think they were rendered with Ulead Media 
>>>> Studio Pro) ... this causes ffmpeg/VLC-player to produce/play 
>>>> corrupted video.
>>>>
>>>> In other players/editors it works fine including:
>>>>
>>>> - VirtualDub
>>>> - Windows Media Player
>>>> - AVCutty
>>>> - Ulead Media Studio Pro (very old)
>>>>
>>>> I had a look at VirtualDub ... there the PAL/NTSC detection is based 
>>>> on the frame rate from AVISTREAMINFO header (dwRate/dwScale) - see 
>>>> 
>> 
> https://github.com/Pavuucek/VirtualDub/blob/f47ebd2536f0034b048180d0b9cb9bde0ab10c73/src/VirtualDub/source/VideoSource.cpp#L1211
>>>>
>>>> As I don't know, how to access the AVI header info inside 
>>>> dvvideo_decode_frame()/ff_dv_frame_profile(), I tried another 
>>>> workaround by checking the buf_size against the dv_profile.
>>>>
>>>> It works fine now, but I don't know, if this is really the best 
>>>> solution ...
>>>
>>> --- a/libavcodec/dv_profile.c
>>> +++ b/libavcodec/dv_profile.c
>>> @@ -281,6 +281,10 @@ const AVDVProfile* 
>>> ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile
>>>         && codec->coded_height==576)
>>>          return &dv_profiles[1];
>>>
>>> +    /* hack for trac issue #8333, dv files with wrong dsf flag - 
>>> detect via buf_size */
>>> +    if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size 
>>> == dv_profiles[1].frame_size)
>>> +        return &dv_profiles[1];
>>> +
>>>
>>> If possible, then it is probably better to move this fallback to later 
>>> in the code, right after the hack for trac issue #217, so previous 
>>> hacks won't get broken...
>>>
>>> Regards,
>>> Marton
>>> _______________________________________________
>>> 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".
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
Mark Plomer March 18, 2021, 1:13 p.m. UTC | #8
Okay cool, I added the check for the PAL flag - this still works fine 
for my test-videos.

I also looked at the old hack above with the codec_tag check - changed 
in 2013 for ticket #2177 (patch by Michael Niedermayer). I removed that 
more specific hack and the test-video attached to ticket #2177 also runs 
into the new hack now ... and still works fine. It also has a "dsf = 0" 
flag, though it is PAL.

So it was really a good idea to consolidate those hacks! Maybe the whole 
generic matching logic should be refactored, to match against the pal 
flag instead of the dsf flag? (The "buf_size" check may also be obsolete 
then). But this will be another task ;-)

I attached the updated patch.

Regards
Mark

Am 16.03.21 um 23:12 schrieb Marton Balint:
>
>> Then at least check the 50/60 flag as well in the VAUX source pack, 
>> e.g.:
>>
>> pal   = !!(frame[80 * 5 + 48 + 3] & 0x20);
>
> And as far as I see, the more specific hack checking for codec_tag and 
> similar can be removed then, because your check covers that case as well.
Marton Balint March 18, 2021, 8:55 p.m. UTC | #9
On Thu, 18 Mar 2021, Mark Plomer wrote:

> Okay cool, I added the check for the PAL flag - this still works fine for my 
> test-videos.
>
> I also looked at the old hack above with the codec_tag check - changed in 
> 2013 for ticket #2177 (patch by Michael Niedermayer). I removed that more 
> specific hack and the test-video attached to ticket #2177 also runs into the 
> new hack now ... and still works fine. It also has a "dsf = 0" flag, though 
> it is PAL.
>
> So it was really a good idea to consolidate those hacks! Maybe the whole 
> generic matching logic should be refactored, to match against the pal flag 
> instead of the dsf flag? (The "buf_size" check may also be obsolete then). 
> But this will be another task ;-)
>
> I attached the updated patch.

It looks good to me, I will apply in a couple of days if nobody comments 
until then. Also Cc'd Dave, as he got probably the most experience with 
dv.

Regards,
Marton
Mark Plomer March 30, 2021, 7:48 a.m. UTC | #10
ping ;-)

Am 18.03.21 um 21:55 schrieb Marton Balint:
> It looks good to me, I will apply in a couple of days if nobody 
> comments until then. Also Cc'd Dave, as he got probably the most 
> experience with dv.
>
> Regards,
> Marton
Marton Balint March 30, 2021, 7:02 p.m. UTC | #11
On Tue, 30 Mar 2021, Mark Plomer wrote:

> ping ;-)

Applied, thanks.

Marton

>
> Am 18.03.21 um 21:55 schrieb Marton Balint:
>> It looks good to me, I will apply in a couple of days if nobody 
>> comments until then. Also Cc'd Dave, as he got probably the most 
>> experience with dv.
>>
>> Regards,
>> Marton
> _______________________________________________
> 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/libavcodec/dv_profile.c b/libavcodec/dv_profile.c
index 66505c886b..8f9a358032 100644
--- a/libavcodec/dv_profile.c
+++ b/libavcodec/dv_profile.c
@@ -281,6 +281,10 @@  const AVDVProfile* ff_dv_frame_profile(AVCodecContext* codec, const AVDVProfile
        && codec->coded_height==576)
         return &dv_profiles[1];
 
+    /* hack for trac issue #8333, dv files with wrong dsf flag - detect via buf_size */
+    if (dsf == 0 && stype == dv_profiles[1].video_stype && buf_size == dv_profiles[1].frame_size)
+        return &dv_profiles[1];
+
     for (i = 0; i < FF_ARRAY_ELEMS(dv_profiles); i++)
         if (dsf == dv_profiles[i].dsf && stype == dv_profiles[i].video_stype)
             return &dv_profiles[i];