diff mbox

[FFmpeg-devel] avformat/segafilmenc - set keyframe bit correctly

Message ID 8feade47-bb2c-0d30-f77b-2d8381231d28@gmail.com
State Superseded
Headers show

Commit Message

Gyan May 5, 2018, 11:46 a.m. UTC
Since the muxer author hasn't made the change, the patch is submitted.

Reference:

http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228602.html
From 79f87ff264c2989193d5e59da8c5cf285940aa50 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Sat, 5 May 2018 17:04:53 +0530
Subject: [PATCH] avformat/segafilmenc - set keyframe bit correctly

As per
https://web.archive.org/web/20020803104640/http://www.pcisys.net:80/~melanson/codecs/film-format.txt,

the top bit of the info1 chunk is set as 1 for inter-coded frames and 0
otherwise.
---
 libavformat/segafilmenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer May 5, 2018, 11:06 p.m. UTC | #1
On Sat, May 05, 2018 at 05:16:09PM +0530, Gyan Doshi wrote:
> Since the muxer author hasn't made the change, the patch is submitted.
> 
> Reference:
> 
> http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228602.html

>  segafilmenc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 710a71f12fae34d125a755bc0f5b290c6a6018e9  0001-avformat-segafilmenc-set-keyframe-bit-correctly.patch
> From 79f87ff264c2989193d5e59da8c5cf285940aa50 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@gyani.pro>
> Date: Sat, 5 May 2018 17:04:53 +0530
> Subject: [PATCH] avformat/segafilmenc - set keyframe bit correctly
> 
> As per
> https://web.archive.org/web/20020803104640/http://www.pcisys.net:80/~melanson/codecs/film-format.txt,
> 
> the top bit of the info1 chunk is set as 1 for inter-coded frames and 0
> otherwise.
> ---
>  libavformat/segafilmenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
> index 5b0d7e69e8..524230e461 100644
> --- a/libavformat/segafilmenc.c
> +++ b/libavformat/segafilmenc.c
> @@ -69,7 +69,7 @@ static int film_write_packet_to_header(AVFormatContext *format_context, FILMPack
>          info1 = pkt->pts;
>          info2 = pkt->duration;
>          /* The top bit being set indicates a key frame */
> -        if (pkt->keyframe)
> +        if (!pkt->keyframe)
>              info1 |= (1 << 31);
>      }

Fixes to muxers and encoders should always bump the minor or micro version of
the lib.

It would also be ideal if this version is stored in the file, that way
demuxers/decoders can compensate for bugs in the muxer/encoder.
Not sure the format has a place for this?

Thanks

[...]
James Almer May 5, 2018, 11:09 p.m. UTC | #2
On 5/5/2018 8:06 PM, Michael Niedermayer wrote:
> On Sat, May 05, 2018 at 05:16:09PM +0530, Gyan Doshi wrote:
>> Since the muxer author hasn't made the change, the patch is submitted.
>>
>> Reference:
>>
>> http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228602.html
> 
>>  segafilmenc.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 710a71f12fae34d125a755bc0f5b290c6a6018e9  0001-avformat-segafilmenc-set-keyframe-bit-correctly.patch
>> From 79f87ff264c2989193d5e59da8c5cf285940aa50 Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg@gyani.pro>
>> Date: Sat, 5 May 2018 17:04:53 +0530
>> Subject: [PATCH] avformat/segafilmenc - set keyframe bit correctly
>>
>> As per
>> https://web.archive.org/web/20020803104640/http://www.pcisys.net:80/~melanson/codecs/film-format.txt,
>>
>> the top bit of the info1 chunk is set as 1 for inter-coded frames and 0
>> otherwise.
>> ---
>>  libavformat/segafilmenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
>> index 5b0d7e69e8..524230e461 100644
>> --- a/libavformat/segafilmenc.c
>> +++ b/libavformat/segafilmenc.c
>> @@ -69,7 +69,7 @@ static int film_write_packet_to_header(AVFormatContext *format_context, FILMPack
>>          info1 = pkt->pts;
>>          info2 = pkt->duration;
>>          /* The top bit being set indicates a key frame */
>> -        if (pkt->keyframe)
>> +        if (!pkt->keyframe)
>>              info1 |= (1 << 31);
>>      }
> 
> Fixes to muxers and encoders should always bump the minor or micro version of
> the lib.
> 
> It would also be ideal if this version is stored in the file, that way
> demuxers/decoders can compensate for bugs in the muxer/encoder.
> Not sure the format has a place for this?

It's a decades old format used on a few games, it's very likely it doesn't.

A micro bump should be enough.

> 
> Thanks
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer May 5, 2018, 11:16 p.m. UTC | #3
On Sat, May 05, 2018 at 08:09:07PM -0300, James Almer wrote:
> On 5/5/2018 8:06 PM, Michael Niedermayer wrote:
> > On Sat, May 05, 2018 at 05:16:09PM +0530, Gyan Doshi wrote:
> >> Since the muxer author hasn't made the change, the patch is submitted.
> >>
> >> Reference:
> >>
> >> http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228602.html
> > 
> >>  segafilmenc.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 710a71f12fae34d125a755bc0f5b290c6a6018e9  0001-avformat-segafilmenc-set-keyframe-bit-correctly.patch
> >> From 79f87ff264c2989193d5e59da8c5cf285940aa50 Mon Sep 17 00:00:00 2001
> >> From: Gyan Doshi <ffmpeg@gyani.pro>
> >> Date: Sat, 5 May 2018 17:04:53 +0530
> >> Subject: [PATCH] avformat/segafilmenc - set keyframe bit correctly
> >>
> >> As per
> >> https://web.archive.org/web/20020803104640/http://www.pcisys.net:80/~melanson/codecs/film-format.txt,
> >>
> >> the top bit of the info1 chunk is set as 1 for inter-coded frames and 0
> >> otherwise.
> >> ---
> >>  libavformat/segafilmenc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
> >> index 5b0d7e69e8..524230e461 100644
> >> --- a/libavformat/segafilmenc.c
> >> +++ b/libavformat/segafilmenc.c
> >> @@ -69,7 +69,7 @@ static int film_write_packet_to_header(AVFormatContext *format_context, FILMPack
> >>          info1 = pkt->pts;
> >>          info2 = pkt->duration;
> >>          /* The top bit being set indicates a key frame */
> >> -        if (pkt->keyframe)
> >> +        if (!pkt->keyframe)
> >>              info1 |= (1 << 31);
> >>      }
> > 
> > Fixes to muxers and encoders should always bump the minor or micro version of
> > the lib.
> > 
> > It would also be ideal if this version is stored in the file, that way
> > demuxers/decoders can compensate for bugs in the muxer/encoder.
> > Not sure the format has a place for this?
> 
> It's a decades old format used on a few games, it's very likely it doesn't.

yes

its probably possible to detect the muxer bug without it, if someone wants to
implement it


> 
> A micro bump should be enough.

sure

[...]
James Almer May 5, 2018, 11:32 p.m. UTC | #4
On 5/5/2018 8:16 PM, Michael Niedermayer wrote:
> On Sat, May 05, 2018 at 08:09:07PM -0300, James Almer wrote:
>> On 5/5/2018 8:06 PM, Michael Niedermayer wrote:
>>> On Sat, May 05, 2018 at 05:16:09PM +0530, Gyan Doshi wrote:
>>>> Since the muxer author hasn't made the change, the patch is submitted.
>>>>
>>>> Reference:
>>>>
>>>> http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228602.html
>>>
>>>>  segafilmenc.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 710a71f12fae34d125a755bc0f5b290c6a6018e9  0001-avformat-segafilmenc-set-keyframe-bit-correctly.patch
>>>> From 79f87ff264c2989193d5e59da8c5cf285940aa50 Mon Sep 17 00:00:00 2001
>>>> From: Gyan Doshi <ffmpeg@gyani.pro>
>>>> Date: Sat, 5 May 2018 17:04:53 +0530
>>>> Subject: [PATCH] avformat/segafilmenc - set keyframe bit correctly
>>>>
>>>> As per
>>>> https://web.archive.org/web/20020803104640/http://www.pcisys.net:80/~melanson/codecs/film-format.txt,
>>>>
>>>> the top bit of the info1 chunk is set as 1 for inter-coded frames and 0
>>>> otherwise.
>>>> ---
>>>>  libavformat/segafilmenc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
>>>> index 5b0d7e69e8..524230e461 100644
>>>> --- a/libavformat/segafilmenc.c
>>>> +++ b/libavformat/segafilmenc.c
>>>> @@ -69,7 +69,7 @@ static int film_write_packet_to_header(AVFormatContext *format_context, FILMPack
>>>>          info1 = pkt->pts;
>>>>          info2 = pkt->duration;
>>>>          /* The top bit being set indicates a key frame */
>>>> -        if (pkt->keyframe)
>>>> +        if (!pkt->keyframe)
>>>>              info1 |= (1 << 31);
>>>>      }
>>>
>>> Fixes to muxers and encoders should always bump the minor or micro version of
>>> the lib.
>>>
>>> It would also be ideal if this version is stored in the file, that way
>>> demuxers/decoders can compensate for bugs in the muxer/encoder.
>>> Not sure the format has a place for this?
>>
>> It's a decades old format used on a few games, it's very likely it doesn't.
> 
> yes
> 
> its probably possible to detect the muxer bug without it, if someone wants to
> implement it

Paul said that if the first video frame was not marked as keyframe then
the file is invalid, so the demuxer can probably use that and flip the
flag accordingly.

Or someone could write a cinepak parser, which would allow us to ignore
container level parameters. But i don't expect that to ever happen :p

> 
> 
>>
>> A micro bump should be enough.
> 
> sure
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
index 5b0d7e69e8..524230e461 100644
--- a/libavformat/segafilmenc.c
+++ b/libavformat/segafilmenc.c
@@ -69,7 +69,7 @@  static int film_write_packet_to_header(AVFormatContext *format_context, FILMPack
         info1 = pkt->pts;
         info2 = pkt->duration;
         /* The top bit being set indicates a key frame */
-        if (pkt->keyframe)
+        if (!pkt->keyframe)
             info1 |= (1 << 31);
     }