diff mbox

[FFmpeg-devel,1/2] avcodec: add photocd decoder

Message ID 20181219203204.7484-1-onemda@gmail.com
State Superseded
Headers show

Commit Message

Paul B Mahol Dec. 19, 2018, 8:32 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/Makefile     |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h    |   1 +
 libavcodec/codec_desc.c |   7 +
 libavcodec/photocd.c    | 445 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 455 insertions(+)
 create mode 100644 libavcodec/photocd.c

Comments

Peter Ross Dec. 19, 2018, 11:52 p.m. UTC | #1
On Wed, Dec 19, 2018 at 09:32:03PM +0100, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/codec_desc.c |   7 +
>  libavcodec/photocd.c    | 445 ++++++++++++++++++++++++++++++++++++++++

here is a small review.

> +    int      thumbnails;  //* number of thumbnails; 0 for normal image */

'//*' strange comment style

> +static const uint32_t img_start [] = { 0 /*dummy */ , 8192, 47104, 196608 };
> +static const uint16_t def_width [] = { 0 /*dummy */ , 192, 384, 768, 1536, 3072, 6144 };
> +static const uint16_t def_height[] = { 0 /*dummy */ , 128, 256, 512, 1024, 2048, 4096 };

if this was a struct it would be more reable. the array index is the same quality.

struct {
   uint32_t start;
   uint16_t width, height;
} img_info[];

also not obvious to me why the dummy values are needed.
array index is alway take from s->resolution, and that is av_clip'd to [1,5].

> +static int photocd_decode_frame(AVCodecContext *avctx, void *data,
> +                                int *got_frame, AVPacket *avpkt)
> +{
> +    PhotoCDContext *s = avctx->priv_data;
> +    const uint8_t *buf = avpkt->data;
> +    GetByteContext *gb = &s->gb;
> +    AVFrame *p = data;
> +    uint8_t *ptr, *ptr1, *ptr2;
> +    int ret;
> +
> +    if (avpkt->size < 7)
> +        return AVERROR_INVALIDDATA;
> +
> +    bytestream2_init(gb, avpkt->data, avpkt->size);

could delay bytestream2_init further down, as it's not used yet.

> +    if (!strncmp("PCD_OPA", buf, 7)) {
> +        s->thumbnails = AV_RL16(buf + 10);

need to check avpkt->size before buf[10];

> +        av_log(avctx, AV_LOG_WARNING, "this is a thumbnails file, "
> +               "reading first thumbnail only\n");
> +    } else if (avpkt->size < 786432) {
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    s->orientation = s->thumbnails ? buf[12] & 3 : buf[0x48] & 3;

need to check avpkt->size here too.

also where can i find a sample to fuzz test?

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Carl Eugen Hoyos Dec. 19, 2018, 11:57 p.m. UTC | #2
2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:

> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
> +{
> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;

I very much welcome this patch but it appears that the colourspace
conversion is missing that was part of the original patchset=-(

Carl Eugen
Paul B Mahol Dec. 20, 2018, 9:02 a.m. UTC | #3
On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>
>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>> +{
>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>
> I very much welcome this patch but it appears that the colourspace
> conversion is missing that was part of the original patchset=-(

Please refrain from telling me such obvious untrue things.
Nicolas George Dec. 20, 2018, 9:16 a.m. UTC | #4
Paul B Mahol (2018-12-20):
> Please refrain from telling me such obvious untrue things.

How can it be obvious if it is untrue?

Rather than taking it as an attack, I think a better approach would be
to assume the comment was given in good faith and use the single line
you answer to give an useful answer. Human interactions would work much
better that way. Please give it a try.

Regards,
Paul B Mahol Dec. 20, 2018, 9:37 a.m. UTC | #5
On 12/20/18, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (2018-12-20):
>> Please refrain from telling me such obvious untrue things.
>
> How can it be obvious if it is untrue?
>
> Rather than taking it as an attack, I think a better approach would be
> to assume the comment was given in good faith and use the single line
> you answer to give an useful answer. Human interactions would work much
> better that way. Please give it a try.

It is an obvious attack. Old patches were posted separately.
Nicolas George Dec. 20, 2018, 9:40 a.m. UTC | #6
Paul B Mahol (2018-12-20):
> It is an obvious attack.

It was a technical comment wrapped in very polite and welcoming
language. Maybe it was an obvious mistake (but if it is obvious, I
cannot see it), but it certainly was not an attack.

Regards,
Carl Eugen Hoyos Dec. 21, 2018, 3:30 p.m. UTC | #7
2018-12-20 10:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
> On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>
>>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>>> +{
>>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>>
>> I very much welcome this patch but it appears that the colourspace
>> conversion is missing that was part of the original patchset=-(
>
> Please refrain from telling me such obvious untrue things.

I may misunderstand your comment, to clarify I attach a jpg
that shows FFmpeg output on top and reference output below
for the sample image IMG0024.PCD.

Carl Eugen
Paul B Mahol Dec. 21, 2018, 3:43 p.m. UTC | #8
On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-12-20 10:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>> On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>
>>>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>>>> +{
>>>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>>>
>>> I very much welcome this patch but it appears that the colourspace
>>> conversion is missing that was part of the original patchset=-(
>>
>> Please refrain from telling me such obvious untrue things.
>
> I may misunderstand your comment, to clarify I attach a jpg
> that shows FFmpeg output on top and reference output below
> for the sample image IMG0024.PCD.

I will ignore your comments as there is misunderstanding from your side.
Nicolas George Dec. 21, 2018, 3:46 p.m. UTC | #9
Paul B Mahol (2018-12-21):
> I will ignore your comments as there is misunderstanding from your side.

Unacceptable. If somebody has misunderstood something you wrote, then
you need to explain better.
Carl Eugen Hoyos Dec. 21, 2018, 3:47 p.m. UTC | #10
> Am 21.12.2018 um 16:43 schrieb Paul B Mahol <onemda@gmail.com>:
> 
>> On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2018-12-20 10:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>> On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>> 
>>>>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>>>>> +{
>>>>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>>>> 
>>>> I very much welcome this patch but it appears that the colourspace
>>>> conversion is missing that was part of the original patchset=-(
>>> 
>>> Please refrain from telling me such obvious untrue things.
>> 
>> I may misunderstand your comment, to clarify I attach a jpg
>> that shows FFmpeg output on top and reference output below
>> for the sample image IMG0024.PCD.
> 
> I will ignore your comments as there is misunderstanding from your side.

Would you like to clarify?

Carl Eugen
Paul B Mahol Dec. 21, 2018, 4:07 p.m. UTC | #11
On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>
>> Am 21.12.2018 um 16:43 schrieb Paul B Mahol <onemda@gmail.com>:
>>
>>> On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>> 2018-12-20 10:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>>> On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>>>
>>>>>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>>>>>> +{
>>>>>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>>>>>
>>>>> I very much welcome this patch but it appears that the colourspace
>>>>> conversion is missing that was part of the original patchset=-(
>>>>
>>>> Please refrain from telling me such obvious untrue things.
>>>
>>> I may misunderstand your comment, to clarify I attach a jpg
>>> that shows FFmpeg output on top and reference output below
>>> for the sample image IMG0024.PCD.
>>
>> I will ignore your comments as there is misunderstanding from your side.
>
> Would you like to clarify?

The colors that PhotoCD uses predates color space definitions.
There is no nice way to support it so only manual conversion could do it.
Paul B Mahol Dec. 21, 2018, 6:21 p.m. UTC | #12
On 12/21/18, Paul B Mahol <onemda@gmail.com> wrote:
> On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>>
>>> Am 21.12.2018 um 16:43 schrieb Paul B Mahol <onemda@gmail.com>:
>>>
>>>> On 12/21/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> 2018-12-20 10:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>>>> On 12/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>>> 2018-12-19 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>>>>>>
>>>>>>> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
>>>>>>> +{
>>>>>>> +    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
>>>>>>
>>>>>> I very much welcome this patch but it appears that the colourspace
>>>>>> conversion is missing that was part of the original patchset=-(
>>>>>
>>>>> Please refrain from telling me such obvious untrue things.
>>>>
>>>> I may misunderstand your comment, to clarify I attach a jpg
>>>> that shows FFmpeg output on top and reference output below
>>>> for the sample image IMG0024.PCD.
>>>
>>> I will ignore your comments as there is misunderstanding from your side.
>>
>> Would you like to clarify?
>
> The colors that PhotoCD uses predates color space definitions.
> There is no nice way to support it so only manual conversion could do it.
>

I will adjust U and V plane components, and leave Y untouched (result
will be acceptable to me).
Paul B Mahol Dec. 21, 2018, 6:50 p.m. UTC | #13
On 12/20/18, Peter Ross <pross@xvid.org> wrote:
> also where can i find a sample to fuzz test?

You can create small samples with ImageMagick/GraphicsMagick otherwise
use google skills.
Carl Eugen Hoyos Dec. 21, 2018, 8:41 p.m. UTC | #14
2018-12-20 0:52 GMT+01:00, Peter Ross <pross@xvid.org>:

> also where can i find a sample to fuzz test?

See http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket5923/

Carl Eugen
Steinar H. Gunderson Dec. 21, 2018, 9:13 p.m. UTC | #15
On Fri, Dec 21, 2018 at 05:07:45PM +0100, Paul B Mahol wrote:
> The colors that PhotoCD uses predates color space definitions.

Really? It looks fairly well-defined to me, though esoteric
(the gamma ramp is basically like sRGB but with a much bigger constant,
and the 8-bit Y'CbCr scaling seems unusual):

  https://en.wikipedia.org/wiki/Photo_CD#Encoding

FFmpeg doesn't have a good understanding of gamma (it rarely actually
converts between different gamma ramps), but that's not a problem with
PhotoCD per se. I can't find a good reason why FFmpeg could not be
extended with conversions from the PhotoCD color space to sRGB, at least
not if clipping out-of-gamut colors is acceptable.

/* Steinar */
Paul B Mahol Dec. 22, 2018, 8:53 a.m. UTC | #16
On 12/21/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> On Fri, Dec 21, 2018 at 05:07:45PM +0100, Paul B Mahol wrote:
>> The colors that PhotoCD uses predates color space definitions.
>
> Really? It looks fairly well-defined to me, though esoteric
> (the gamma ramp is basically like sRGB but with a much bigger constant,
> and the 8-bit Y'CbCr scaling seems unusual):
>
>   https://en.wikipedia.org/wiki/Photo_CD#Encoding
>
> FFmpeg doesn't have a good understanding of gamma (it rarely actually
> converts between different gamma ramps), but that's not a problem with
> PhotoCD per se. I can't find a good reason why FFmpeg could not be
> extended with conversions from the PhotoCD color space to sRGB, at least
> not if clipping out-of-gamut colors is acceptable.

I'm all ears.
Steinar H. Gunderson Dec. 22, 2018, 7:59 p.m. UTC | #17
On Sat, Dec 22, 2018 at 09:53:16AM +0100, Paul B Mahol wrote:
>> FFmpeg doesn't have a good understanding of gamma (it rarely actually
>> converts between different gamma ramps), but that's not a problem with
>> PhotoCD per se. I can't find a good reason why FFmpeg could not be
>> extended with conversions from the PhotoCD color space to sRGB, at least
>> not if clipping out-of-gamut colors is acceptable.
> I'm all ears.

I happen to have a library that does all of this stuff... :-)
(https://movit.sesse.net/)

I don't think FFmpeg really wants to link in Movit for a variety of reasons,
and in this case, PhotoCD is nominally Rec. 709, so you don't actually need
a colorspace transform. This means that the only steps you really need would
be:

 1. Decode the YCC to RGB. Allow for out-of-0..255 (ideally float, but
    FFmpeg probably wants to use int16 or something similar instead).
 2. Convert to linear gamma (either float, or 16-bit fixed point) using the
    inverse of the function mentioned in Wikipedia.
 3. Convert back from linear gamma to sRGB or Rec. 709 gamma.
 4. Clip to 0..1, then scale to 0..255.

Step 2, 3, 4 can probably be collapsed into a LUT that can be applied three
times (the channels are independent, since the RGB color space is the same).

If you're not happy with blown highlights (colors that clip), it's a much
harder problem.

/* Steinar */
Paul B Mahol Dec. 22, 2018, 8:04 p.m. UTC | #18
On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> On Sat, Dec 22, 2018 at 09:53:16AM +0100, Paul B Mahol wrote:
>>> FFmpeg doesn't have a good understanding of gamma (it rarely actually
>>> converts between different gamma ramps), but that's not a problem with
>>> PhotoCD per se. I can't find a good reason why FFmpeg could not be
>>> extended with conversions from the PhotoCD color space to sRGB, at least
>>> not if clipping out-of-gamut colors is acceptable.
>> I'm all ears.
>
> I happen to have a library that does all of this stuff... :-)
> (https://movit.sesse.net/)
>
> I don't think FFmpeg really wants to link in Movit for a variety of reasons,
> and in this case, PhotoCD is nominally Rec. 709, so you don't actually need
> a colorspace transform. This means that the only steps you really need would
> be:
>
>  1. Decode the YCC to RGB. Allow for out-of-0..255 (ideally float, but
>     FFmpeg probably wants to use int16 or something similar instead).
>  2. Convert to linear gamma (either float, or 16-bit fixed point) using the
>     inverse of the function mentioned in Wikipedia.
>  3. Convert back from linear gamma to sRGB or Rec. 709 gamma.
>  4. Clip to 0..1, then scale to 0..255.
>
> Step 2, 3, 4 can probably be collapsed into a LUT that can be applied three
> times (the channels are independent, since the RGB color space is the same).
>
> If you're not happy with blown highlights (colors that clip), it's a much
> harder problem.

I can not accept internal conversion to RGB. This is subsampled format
after all.
Steinar H. Gunderson Dec. 22, 2018, 8:14 p.m. UTC | #19
On Sat, Dec 22, 2018 at 09:04:26PM +0100, Paul B Mahol wrote:
> I can not accept internal conversion to RGB. This is subsampled format
> after all.

Well, it's not Y'CbCr, so if so, you'd need to add a new AVPixelFormat value
(e.g. AV_PIX_FMT_YCC420P). I'm not sure what applications would do with it
except convert it to RGB, though. (You can't convert subsampled YCC to
subsampled Y'CbCr without upsampling, converting and then subsampling again,
which is obviously lossy.)

/* Steinar */
Paul B Mahol Dec. 22, 2018, 8:18 p.m. UTC | #20
On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> On Sat, Dec 22, 2018 at 09:04:26PM +0100, Paul B Mahol wrote:
>> I can not accept internal conversion to RGB. This is subsampled format
>> after all.
>
> Well, it's not Y'CbCr, so if so, you'd need to add a new AVPixelFormat value
> (e.g. AV_PIX_FMT_YCC420P). I'm not sure what applications would do with it
> except convert it to RGB, though. (You can't convert subsampled YCC to
> subsampled Y'CbCr without upsampling, converting and then subsampling again,
> which is obviously lossy.)

Unacceptable, I'm not adding another yuv420p variant.
Steinar H. Gunderson Dec. 22, 2018, 8:28 p.m. UTC | #21
On Sat, Dec 22, 2018 at 09:18:11PM +0100, Paul B Mahol wrote:
> Unacceptable, I'm not adding another yuv420p variant.

Well, if returning YCC as YCC is unacceptable, and converting YCC to RGB is
unacceptable, I believe your only choices are:

  1. Try to convert YCC to Y'CbCr ignoring the gamma curve, which will return
     images that look wrong and cannot be repaired by reasonable means.
  2. Return YCC mislabeled as something else, which will look even more wrong.
  3. Don't include PhotoCD support in FFmpeg.

I don't know if any of these options are acceptable to you.

/* Steinar */
Paul B Mahol Dec. 22, 2018, 8:32 p.m. UTC | #22
On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> On Sat, Dec 22, 2018 at 09:18:11PM +0100, Paul B Mahol wrote:
>> Unacceptable, I'm not adding another yuv420p variant.
>
> Well, if returning YCC as YCC is unacceptable, and converting YCC to RGB is
> unacceptable, I believe your only choices are:
>
>   1. Try to convert YCC to Y'CbCr ignoring the gamma curve, which will
> return
>      images that look wrong and cannot be repaired by reasonable means.
>   2. Return YCC mislabeled as something else, which will look even more
> wrong.
>   3. Don't include PhotoCD support in FFmpeg.
>

4. Leave user to do conversion as he wish.
Steinar H. Gunderson Dec. 22, 2018, 8:34 p.m. UTC | #23
On Sat, Dec 22, 2018 at 09:32:35PM +0100, Paul B Mahol wrote:
>>   2. Return YCC mislabeled as something else, which will look even more wrong.
> 4. Leave user to do conversion as he wish.

That's essentially the same as #2, no?

/* Steinar */
Michael Niedermayer Dec. 23, 2018, 11:08 a.m. UTC | #24
On Sat, Dec 22, 2018 at 09:28:47PM +0100, Steinar H. Gunderson wrote:
> On Sat, Dec 22, 2018 at 09:18:11PM +0100, Paul B Mahol wrote:
> > Unacceptable, I'm not adding another yuv420p variant.
> 
> Well, if returning YCC as YCC is unacceptable, and converting YCC to RGB is
> unacceptable, I believe your only choices are:
> 
>   1. Try to convert YCC to Y'CbCr ignoring the gamma curve, which will return
>      images that look wrong and cannot be repaired by reasonable means.
>   2. Return YCC mislabeled as something else, which will look even more wrong.
>   3. Don't include PhotoCD support in FFmpeg.

a user switch could be added to choose between RGB and incorrect YCbCr
the alternative would be adding support to swscale for the new variant
and some means to transport the information to it so its converted
correctly. for example by using AVColor* enums

I think doing this in swscale is the "proper" solution but doing it in
the decoder is not that unreasonable as this is a rather odd format.
Also another pixfmt is not ideal as paul already mentioned. Though i
would not outright reject that option, its probably easier to do than
using AVColor* enums and have this all working well

either way, iam fine with whatever others are fine with ...

Thanks

[...]
Carl Eugen Hoyos Jan. 11, 2019, 2:36 a.m. UTC | #25
2018-12-22 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
> On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
>> On Sat, Dec 22, 2018 at 09:18:11PM +0100, Paul B Mahol wrote:
>>> Unacceptable, I'm not adding another yuv420p variant.
>>
>> Well, if returning YCC as YCC is unacceptable, and converting YCC to RGB
>> is
>> unacceptable, I believe your only choices are:
>>
>>   1. Try to convert YCC to Y'CbCr ignoring the gamma curve, which will
>> return
>>      images that look wrong and cannot be repaired by reasonable means.
>>   2. Return YCC mislabeled as something else, which will look even more
>> wrong.
>>   3. Don't include PhotoCD support in FFmpeg.
>>
>
> 4. Leave user to do conversion as he wish.

Could you commit something with "-strict experimental"?

Carl Eugen
Paul B Mahol Jan. 11, 2019, 9:25 a.m. UTC | #26
On 1/11/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-12-22 21:32 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
>> On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
>>> On Sat, Dec 22, 2018 at 09:18:11PM +0100, Paul B Mahol wrote:
>>>> Unacceptable, I'm not adding another yuv420p variant.
>>>
>>> Well, if returning YCC as YCC is unacceptable, and converting YCC to RGB
>>> is
>>> unacceptable, I believe your only choices are:
>>>
>>>   1. Try to convert YCC to Y'CbCr ignoring the gamma curve, which will
>>> return
>>>      images that look wrong and cannot be repaired by reasonable means.
>>>   2. Return YCC mislabeled as something else, which will look even more
>>> wrong.
>>>   3. Don't include PhotoCD support in FFmpeg.
>>>
>>
>> 4. Leave user to do conversion as he wish.
>
> Could you commit something with "-strict experimental"?

No, pay me first.
Nicolas George Jan. 11, 2019, 3:20 p.m. UTC | #27
Paul B Mahol (12019-01-11):
> No, pay me first.

That actually explains a lot of things...

But I wonder: were you not payed by somebody for this unfinished
demuxer?
Carl Eugen Hoyos Jan. 11, 2019, 3:23 p.m. UTC | #28
2019-01-11 16:20 GMT+01:00, Nicolas George <george@nsup.org>:
> Paul B Mahol (12019-01-11):
>> No, pay me first.
>
> That actually explains a lot of things...
>
> But I wonder: were you not payed by somebody for this
> unfinished demuxer?

The original unfinished demuxer was posted to the development
mailing list:
https://ffmpeg.org/pipermail/ffmpeg-devel/2010-January/086658.html

Carl Eugen
Nicolas George Jan. 11, 2019, 3:26 p.m. UTC | #29
Carl Eugen Hoyos (12019-01-11):
> The original unfinished demuxer was posted to the development
> mailing list:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2010-January/086658.html

Ah, good to know. Still, Paul made efforts to resurrect that patch, I
wonder if these efforts were sponsored, and in that case what the
sponsor thinks about them staying unfinished, or if they were not, and
in that case why expect payment for the final efforts.

More generally, I think it would be a good practice that any submission
that is sponsored is clearly marked as such: identity of the sponsor,
amount, identity of the recipient(s).

Regards,
Carl Eugen Hoyos Jan. 11, 2019, 3:28 p.m. UTC | #30
2019-01-11 16:26 GMT+01:00, Nicolas George <george@nsup.org>:
> Carl Eugen Hoyos (12019-01-11):
>> The original unfinished demuxer was posted to the development
>> mailing list:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2010-January/086658.html
>
> Ah, good to know. Still, Paul made efforts to resurrect that patch, I
> wonder if these efforts were sponsored, and in that case what the
> sponsor thinks about them staying unfinished, or if they were not, and
> in that case why expect payment for the final efforts.
>
> More generally, I think it would be a good practice that any submission
> that is sponsored is clearly marked as such: identity of the sponsor,
> amount, identity of the recipient(s).

I believe amount is never published (so far at least afair).

Carl Eugen
Derek Buitenhuis Jan. 11, 2019, 3:29 p.m. UTC | #31
On 11/01/2019 15:26, Nicolas George wrote:
> Carl Eugen Hoyos (12019-01-11):
>> The original unfinished demuxer was posted to the development
>> mailing list:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2010-January/086658.html
> 
> Ah, good to know. Still, Paul made efforts to resurrect that patch, I
> wonder if these efforts were sponsored, and in that case what the
> sponsor thinks about them staying unfinished, or if they were not, and
> in that case why expect payment for the final efforts.

Irrelevant to whether a patch is acceptable or not to FFmpeg.

> More generally, I think it would be a good practice that any submission
> that is sponsored is clearly marked as such: identity of the sponsor,
> amount, identity of the recipient(s).

Why and to what end? Seems like a great way to witchhunt specific patches.

Also, nobody has ANY business knowing what someone else is paid if they don't
want to supply that info. None at all.

- Derek
Nicolas George Jan. 11, 2019, 3:31 p.m. UTC | #32
Carl Eugen Hoyos (12019-01-11):
> I believe amount is never published (so far at least afair).

I think it should. I wonder what other people think.

Regards,
Kieran Kunhya Jan. 11, 2019, 4:13 p.m. UTC | #33
On Fri, 11 Jan 2019 at 15:31 Nicolas George <george@nsup.org> wrote:

> Carl Eugen Hoyos (12019-01-11):
> > I believe amount is never published (so far at least afair).
>
> I think it should. I wonder what other people think.
>

Are you completely out of your mind?

Regards,
Kieran Kunhya
Nicolas George Jan. 11, 2019, 4:23 p.m. UTC | #34
Derek Buitenhuis (12019-01-11):
> Irrelevant to whether a patch is acceptable or not to FFmpeg.

In theory, it is, and it should be.

In practice, I have several times suspected a sponsored work was the
reason behind cutting corners, poor design and future planning, bad
reception of review comments, etc. A mandatory disclosure would make
these bad behaviours obvious.

I will produce a patch on the developer policy to start a real
discussion.

Regards,
Derek Buitenhuis Jan. 11, 2019, 4:27 p.m. UTC | #35
On 11/01/2019 16:23, Nicolas George wrote:
> Derek Buitenhuis (12019-01-11):
>> Irrelevant to whether a patch is acceptable or not to FFmpeg.
> 
> In theory, it is, and it should be.
> 
> In practice, I have several times suspected a sponsored work was the
> reason behind cutting corners, poor design and future planning, bad
> reception of review comments, etc. A mandatory disclosure would make
> these bad behaviours obvious.

I'm sorry, but this just reeks of "your motives aren't pure enough for
us to trust you" partisanship, which is, frankly, disgusting.

> I will produce a patch on the developer policy to start a real
> discussion.

I suspect the replies will not reflect what you think they will, but
if it is the only way... so be it.

- Derek
Nicolas George Jan. 11, 2019, 4:30 p.m. UTC | #36
Derek Buitenhuis (12019-01-11):
> I'm sorry, but this just reeks of "your motives aren't pure enough for
> us to trust you" partisanship, which is, frankly, disgusting.

Maybe not enough people have been obnoxious to you during review for you
to feel the problem. Or maybe you have thicker skin.

> I suspect the replies will not reflect what you think they will, but
> if it is the only way... so be it.

That is possible. Let us see.

Regards,
Kieran O Leary Jan. 11, 2019, 5:41 p.m. UTC | #37
On Fri, 11 Jan 2019, 15:31 Nicolas George <george@nsup.org wrote:

> Carl Eugen Hoyos (12019-01-11):
> > I believe amount is never published (so far at least afair).
>
> I think it should. I wonder what other people think.
>

I don't see why this declaration is helpful or necessary in any way. Can
you elaborate on why it is?

Best,

Kieran
Vittorio Giovara April 7, 2020, 10:48 a.m. UTC | #38
On Sat, Dec 22, 2018 at 3:18 PM Paul B Mahol <onemda@gmail.com> wrote:

> On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> > On Sat, Dec 22, 2018 at 09:04:26PM +0100, Paul B Mahol wrote:
> >> I can not accept internal conversion to RGB. This is subsampled format
> >> after all.
> >
> > Well, it's not Y'CbCr, so if so, you'd need to add a new AVPixelFormat
> value
> > (e.g. AV_PIX_FMT_YCC420P). I'm not sure what applications would do with
> it
> > except convert it to RGB, though. (You can't convert subsampled YCC to
> > subsampled Y'CbCr without upsampling, converting and then subsampling
> again,
> > which is obviously lossy.)
>
> Unacceptable, I'm not adding another yuv420p variant.
>

Agreed, no need for another pixel format variant, but maybe you could just
leave the buffer as is and tag the color transfer with
AVCOL_TRC_IEC61966_2_1?
Vittorio Giovara April 7, 2020, 10:10 p.m. UTC | #39
On Tue, Apr 7, 2020 at 7:58 AM Paul B Mahol <onemda@gmail.com> wrote:

> On 4/7/20, Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
> > On Sat, Dec 22, 2018 at 3:18 PM Paul B Mahol <onemda@gmail.com> wrote:
> >
> >> On 12/22/18, Steinar H. Gunderson <steinar+ffmpeg@gunderson.no> wrote:
> >> > On Sat, Dec 22, 2018 at 09:04:26PM +0100, Paul B Mahol wrote:
> >> >> I can not accept internal conversion to RGB. This is subsampled
> format
> >> >> after all.
> >> >
> >> > Well, it's not Y'CbCr, so if so, you'd need to add a new AVPixelFormat
> >> value
> >> > (e.g. AV_PIX_FMT_YCC420P). I'm not sure what applications would do
> with
> >> it
> >> > except convert it to RGB, though. (You can't convert subsampled YCC to
> >> > subsampled Y'CbCr without upsampling, converting and then subsampling
> >> again,
> >> > which is obviously lossy.)
> >>
> >> Unacceptable, I'm not adding another yuv420p variant.
> >>
> >
> > Agreed, no need for another pixel format variant, but maybe you could
> just
> > leave the buffer as is and tag the color transfer with
> > AVCOL_TRC_IEC61966_2_1?
>
> Hmm, you sure that thing is exactly same what photocd uses?
> Is there a way to check it, in way that there is tool that converts
> that one to 709?
>

Sorry I'm unfamiliar with photocd to answer that, I just saw YCC mentioned
and remembered I saw it eslewhere.
If you have the tools to verify correct output, you could try with with
zimg/zscale, as it's one of the supported transfer.
Just make sure swscale is not inserted randomly in the filter chain.
diff mbox

Patch

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index d53b8ff330..d32761559e 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -508,6 +508,7 @@  OBJS-$(CONFIG_PGM_ENCODER)             += pnmenc.o
 OBJS-$(CONFIG_PGMYUV_DECODER)          += pnmdec.o pnm.o
 OBJS-$(CONFIG_PGMYUV_ENCODER)          += pnmenc.o
 OBJS-$(CONFIG_PGSSUB_DECODER)          += pgssubdec.o
+OBJS-$(CONFIG_PHOTOCD_DECODER)         += photocd.o
 OBJS-$(CONFIG_PICTOR_DECODER)          += pictordec.o cga_data.o
 OBJS-$(CONFIG_PIXLET_DECODER)          += pixlet.o
 OBJS-$(CONFIG_PJS_DECODER)             += textdec.o ass.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d70646e91a..db2071f980 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -227,6 +227,7 @@  extern AVCodec ff_pgm_encoder;
 extern AVCodec ff_pgm_decoder;
 extern AVCodec ff_pgmyuv_encoder;
 extern AVCodec ff_pgmyuv_decoder;
+extern AVCodec ff_photocd_decoder;
 extern AVCodec ff_pictor_decoder;
 extern AVCodec ff_pixlet_decoder;
 extern AVCodec ff_png_encoder;
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 3922e89331..ffa3a5ddbd 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -452,6 +452,7 @@  enum AVCodecID {
     AV_CODEC_ID_MWSC,
     AV_CODEC_ID_WCMV,
     AV_CODEC_ID_RASC,
+    AV_CODEC_ID_PHOTOCD,
 
     /* various PCM "codecs" */
     AV_CODEC_ID_FIRST_AUDIO = 0x10000,     ///< A dummy id pointing at the start of audio codecs
diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
index 73f343ce24..0af51e5c2f 100644
--- a/libavcodec/codec_desc.c
+++ b/libavcodec/codec_desc.c
@@ -1691,6 +1691,13 @@  static const AVCodecDescriptor codec_descriptors[] = {
         .long_name = NULL_IF_CONFIG_SMALL("RemotelyAnywhere Screen Capture"),
         .props     = AV_CODEC_PROP_LOSSY,
     },
+    {
+        .id        = AV_CODEC_ID_PHOTOCD,
+        .type      = AVMEDIA_TYPE_VIDEO,
+        .name      = "photocd",
+        .long_name = NULL_IF_CONFIG_SMALL("Kodak Photo CD"),
+        .props     = AV_CODEC_PROP_LOSSY,
+    },
 
     /* various PCM "codecs" */
     {
diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c
new file mode 100644
index 0000000000..043fd28533
--- /dev/null
+++ b/libavcodec/photocd.c
@@ -0,0 +1,445 @@ 
+/*
+ * Kodak PhotoCD (a.k.a. ImagePac) image decoder
+ *
+ * Copyright (c) 1996-2002 Gerd Knorr
+ * Copyright (c) 2010 Kenneth Vermeirsch
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * Kodak PhotoCD (a.k.a. ImagePac) image decoder
+ *
+ * Supports resolutions up to 3072x2048.
+ */
+
+#include "libavutil/avassert.h"
+#include "libavutil/intreadwrite.h"
+#include "libavutil/opt.h"
+#include "avcodec.h"
+#include "bytestream.h"
+#include "get_bits.h"
+#include "internal.h"
+
+typedef struct PhotoCDContext {
+    AVClass *class;
+    int      lowres;
+
+    GetByteContext gb;
+    int      thumbnails;  //* number of thumbnails; 0 for normal image */
+    int      resolution;
+    int      orientation;
+
+    const uint8_t *stream;
+    int      streampos;
+
+    uint8_t *seq [3]; //* huffman tables */
+    uint8_t *bits[3];
+} PhotoCDContext;
+
+static const uint32_t img_start [] = { 0 /*dummy */ , 8192, 47104, 196608 };
+static const uint16_t def_width [] = { 0 /*dummy */ , 192, 384, 768, 1536, 3072, 6144 };
+static const uint16_t def_height[] = { 0 /*dummy */ , 128, 256, 512, 1024, 2048, 4096 };
+
+#define HTABLE_SIZE 0x10000
+
+static av_always_inline void interp_lowres(PhotoCDContext *s, AVFrame *picture,
+                                           int width, int height)
+{
+    GetByteContext *gb = &s->gb;
+    int start = s->streampos + img_start[3];
+    uint8_t *ptr, *ptr1, *ptr2;
+    uint8_t *dest;
+    int fill;
+
+    ptr  = picture->data[0];
+    ptr1 = picture->data[1];
+    ptr2 = picture->data[2];
+
+    bytestream2_seek(gb, start, SEEK_SET);
+
+    for (int y = 0; y < height; y += 2) {
+        dest = ptr;
+        for (int x = 0; x < width - 1; x++) {
+            fill = bytestream2_get_byte(gb);
+            *(dest++) = fill;
+            *(dest++) = (fill + bytestream2_peek_byte(gb) + 1) >> 1;
+        }
+        fill      = bytestream2_get_byte(gb);
+        *(dest++) = fill;
+        *(dest++) = fill;
+
+        ptr += picture->linesize[0] << 1;
+
+        dest = ptr;
+        for (int x = 0; x < width - 1; x++) {
+            fill = bytestream2_get_byte(gb);
+            *(dest++) =  fill;
+            *(dest++) = (fill + bytestream2_peek_byte(gb) + 1) >> 1;
+        }
+        fill      = bytestream2_get_byte(gb);
+        *(dest++) = fill;
+        *(dest++) = fill;
+
+        ptr += picture->linesize[0] << 1;
+
+        dest = ptr1;
+        for (int x = 0; x < (width >> 1) - 1; x++) {
+            fill = bytestream2_get_byte(gb);
+            *(dest++) =  fill;
+            *(dest++) = (fill + bytestream2_peek_byte(gb) + 1) >> 1;
+        }
+        fill      = bytestream2_get_byte(gb);
+        *(dest++) = fill;
+        *(dest++) = fill;
+
+        ptr1 += picture->linesize[1] << 1;
+
+        dest = ptr2;
+        for (int x = 0; x < (width >> 1) - 1; x++) {
+            fill = bytestream2_get_byte(gb);
+            *(dest++) =  fill;
+            *(dest++) = (fill + bytestream2_peek_byte(gb) + 1) >> 1;
+        }
+        fill      = bytestream2_get_byte(gb);
+        *(dest++) = fill;
+        *(dest++) = fill;
+
+        ptr2 += picture->linesize[2] << 1;
+    }
+
+    s->streampos += bytestream2_tell(gb) - start;
+}
+
+static void interp_lines(uint8_t *ptr, int linesize, int width, int height)
+{
+    uint8_t *src1, *src2, *dest;
+    int x;
+
+    for (int y = 0; y < height - 2; y += 2) {
+        src1 = ptr;
+        dest = src1 + linesize;
+        src2 = dest + linesize;
+        for (x = 0; x < width - 2; x += 2) {
+            dest[x]     = (src1[x] + src2[x] + 1) >> 1;
+            dest[x + 1] = (src1[x] + src2[x] + src1[x + 2] + src2[x + 2] + 2) >> 2;
+        }
+        dest[x] = dest[x + 1] = (src1[x] + src2[x] + 1) >> 1;
+
+        ptr += linesize << 1;
+    }
+
+    src1 = ptr;
+    dest = ptr + linesize;
+    for (x = 0; x < width - 2; x += 2) {
+        dest[x]     = src1[x];
+        dest[x + 1] = (src1[x] + src1[x + 2] + 1) >> 1;
+    }
+    dest[x] = dest[x + 1] = src1[x];
+}
+
+static void interp_pixels(uint8_t *ptr, int linesize, int width, int height)
+{
+    uint8_t *src, *dest;
+
+    for (int y = height - 2; y >= 0; y -= 2) {
+        src  = ptr + (y >> 1) * linesize;
+        dest = ptr +  y       * linesize;
+
+        dest[width - 2] = dest[width - 1] = src[(width >> 1) - 1];
+        for (int x = width - 4; x >= 0; x -= 2) {
+            dest[x]     =  src[x >> 1];
+            dest[x + 1] = (src[x >> 1] + src[(x >> 1) + 1] + 1) >> 1;
+        }
+    }
+}
+
+static int read_hufftable(AVCodecContext *avctx, uint8_t **aseq, uint8_t **abits)
+{
+    PhotoCDContext *s = avctx->priv_data;
+    GetByteContext *gb = &s->gb;
+    int start = s->streampos;
+    int len, j;
+
+    bytestream2_seek(gb, start, SEEK_SET);
+
+    len = bytestream2_get_byte(gb);
+
+    if (!*aseq)
+        *aseq  = av_malloc(HTABLE_SIZE * sizeof(uint8_t));
+    if (!*abits)
+        *abits = av_malloc(HTABLE_SIZE * sizeof(uint8_t));
+
+    memset(*aseq,  0, HTABLE_SIZE * sizeof(uint8_t));
+    memset(*abits, 0, HTABLE_SIZE * sizeof(uint8_t));
+
+    for (; len >= 0; len--) {
+        const int bits = bytestream2_get_byte(gb) + 1;
+        const int seq  = bytestream2_get_be16(gb);
+        const int seq2 = seq + (0x10000 >> bits);
+        const int fill = bytestream2_get_byte(gb);
+        for (j = seq; j < seq2; j++) {
+            if ((*abits)[j]) {
+                av_log(avctx, AV_LOG_ERROR, "invalid huffmann code table #%x\n", j);
+                return -1;
+            }
+            (*aseq )[j] = fill;
+            (*abits)[j] = bits;
+        }
+    }
+
+    s->streampos = bytestream2_tell(gb);
+    return 0;
+}
+
+static int decode_huff(AVCodecContext *avctx, AVFrame *frame,
+                       int target_res, int curr_res)
+{
+    PhotoCDContext *s = avctx->priv_data;
+    GetBitContext g;
+    GetByteContext *gb = &s->gb;
+    int y = 0, type, height, y2;
+    int start = s->streampos;
+    unsigned shiftreg, bit;
+    const int scaling = target_res - curr_res;
+    const uint8_t type2idx[] = { 0, 0xff, 1, 2 };
+
+    bytestream2_seek(gb, start, SEEK_SET);
+    init_get_bits8(&g, gb->buffer, bytestream2_get_bytes_left(gb));
+
+    height = def_height[curr_res];
+    y2 = avctx->height >> scaling;
+
+    while (y < height) {
+        uint8_t *data;
+        uint8_t *seq;
+        uint8_t *bits;
+        int x2, idx;
+
+        bit = 0;
+        for (; get_bits_left(&g) > 0;) {
+            if ((show_bits_long(&g, 32) & 0x00fff000) == 0xfff000)
+                break;
+            skip_bits(&g, 8);
+        }
+
+        shiftreg = show_bits_long(&g, 32) & 0xffffff00;
+        while (shiftreg != 0xfffffe00) {
+            if (get_bits_left(&g) <= 0)
+                return -1;
+            skip_bits(&g, 1);
+            shiftreg = show_bits_long(&g, 32) & 0xffffff00;
+        }
+        skip_bits(&g, 16);
+        y = (show_bits_long(&g, 32) >> 9) & 0x1fff;
+        skip_bits(&g, 8);
+        if (y >= height)
+            break;
+        type = show_bits_long(&g, 32) >> 30;
+        skip_bits(&g, 16);
+
+        if (type == 1)
+            return -1;
+        idx  = type2idx[type];
+        seq  = s->seq [idx];
+        bits = s->bits[idx];
+        if (!bits)
+            return -1;
+        if (!seq)
+            return -1;
+
+        data = frame->data[idx] + (y >> !!type) * frame->linesize[idx];
+
+        x2 = avctx->width >> (scaling + !!type);
+        for (int x = 0; x < x2; x++) {
+            if (get_bits_left(&g) <= 0)
+                return -1;
+            int n = show_bits_long(&g, 32) & 0xffff;
+            int b = bits[n];
+            int m = show_bits_long(&g, 32) & (1 << b);
+            skip_bits_long(&g, b);
+            data[x] = av_clip_uint8((int)data[x] + (int)seq[m]);
+        }
+    }
+
+    s->streampos += (get_bits_count(&g) + 7) >> 3;
+    s->streampos  = (s->streampos + 0x6000 + 2047) & ~0x7ff;
+
+    return 0;
+}
+
+static int photocd_decode_frame(AVCodecContext *avctx, void *data,
+                                int *got_frame, AVPacket *avpkt)
+{
+    PhotoCDContext *s = avctx->priv_data;
+    const uint8_t *buf = avpkt->data;
+    GetByteContext *gb = &s->gb;
+    AVFrame *p = data;
+    uint8_t *ptr, *ptr1, *ptr2;
+    int ret;
+
+    if (avpkt->size < 7)
+        return AVERROR_INVALIDDATA;
+
+    bytestream2_init(gb, avpkt->data, avpkt->size);
+
+    if (!strncmp("PCD_OPA", buf, 7)) {
+        s->thumbnails = AV_RL16(buf + 10);
+        av_log(avctx, AV_LOG_WARNING, "this is a thumbnails file, "
+               "reading first thumbnail only\n");
+    } else if (avpkt->size < 786432) {
+        return AVERROR_INVALIDDATA;
+    }
+
+    s->orientation = s->thumbnails ? buf[12] & 3 : buf[0x48] & 3;
+
+    if (s->thumbnails)
+        s->resolution = 1;
+    else if (avpkt->size <= 788480)
+        s->resolution = 3;
+    else
+        s->resolution = av_clip(5 - s->lowres, 1, 5);
+
+    avctx->width  = def_width [s->resolution];
+    avctx->height = def_height[s->resolution];
+
+    if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
+        return ret;
+    p->pict_type = AV_PICTURE_TYPE_I;
+    p->key_frame = 1;
+
+    if (s->resolution < 4) {
+        ptr  = p->data[0];
+        ptr1 = p->data[1];
+        ptr2 = p->data[2];
+
+        if (s->thumbnails) {
+            bytestream2_seek(gb, 10240, SEEK_SET);
+        } else {
+            bytestream2_seek(gb, img_start[s->resolution], SEEK_SET);
+        }
+
+        for (int y = 0; y < avctx->height; y += 2) {
+            bytestream2_get_buffer(gb, ptr, avctx->width);
+            ptr += p->linesize[0];
+
+            bytestream2_get_buffer(gb, ptr, avctx->width);
+            ptr += p->linesize[0];
+
+            bytestream2_get_buffer(gb, ptr1, avctx->width >> 1);
+            ptr1 += p->linesize[1];
+
+            bytestream2_get_buffer(gb, ptr2, avctx->width >> 1);
+            ptr2 += p->linesize[2];
+        }
+    } else {
+        s->stream = buf;
+        s->streampos = 0;
+        ptr  = p->data[0];
+        ptr1 = p->data[1];
+        ptr2 = p->data[2];
+
+        interp_lowres(s, p, def_width[3], def_height[3]);
+
+        interp_lines(ptr1, p->linesize[1], def_width[3], def_height[3]);
+        interp_lines(ptr2, p->linesize[2], def_width[3], def_height[3]);
+
+        if (s->resolution == 5) {
+            interp_pixels(ptr1, p->linesize[1], def_width[4], def_height[4]);
+            interp_lines (ptr1, p->linesize[1], def_width[4], def_height[4]);
+            interp_pixels(ptr2, p->linesize[2], def_width[4], def_height[4]);
+            interp_lines (ptr2, p->linesize[2], def_width[4], def_height[4]);
+        }
+
+        interp_lines(ptr, p->linesize[0], def_width[4], def_height[4]);
+
+        s->streampos = 0xc2000;
+        if (read_hufftable(avctx, &s->seq[0], &s->bits[0]) < 0)
+            return AVERROR_INVALIDDATA;
+        s->streampos = (s->streampos + 2047) & ~0x3ff;
+        if (decode_huff(avctx, p, s->resolution, 4) < 0)
+            return AVERROR_INVALIDDATA;
+
+        if (s->resolution == 5) {
+            interp_pixels(ptr, p->linesize[0], def_width[5], def_height[5]);
+            interp_lines (ptr, p->linesize[0], def_width[5], def_height[5]);
+
+            for (int n = 0; n < 3; n++) {
+                if (read_hufftable(avctx, &s->seq[n], &s->bits[n]) < 0)
+                    return AVERROR_INVALIDDATA;
+            }
+            s->streampos = (s->streampos + 2047) & ~0x3ff;
+            if (decode_huff(avctx, p, 5, 5) < 0)
+                return AVERROR_INVALIDDATA;
+        }
+    }
+
+    *got_frame = 1;
+
+    return bytestream2_tell(gb);
+}
+
+static av_cold int photocd_decode_init(AVCodecContext *avctx)
+{
+    avctx->pix_fmt    = AV_PIX_FMT_YUV420P;
+
+    return 0;
+}
+
+static av_cold int photocd_decode_close(AVCodecContext *avctx)
+{
+    PhotoCDContext *s = avctx->priv_data;
+
+    av_free(s->seq [0]);
+    av_free(s->bits[0]);
+    av_free(s->seq [1]);
+    av_free(s->bits[1]);
+    av_free(s->seq [2]);
+    av_free(s->bits[2]);
+
+    return 0;
+}
+
+#define OFFSET(x) offsetof(PhotoCDContext, x)
+#define VD AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM
+
+static const AVOption options[] = {
+    { "lowres",  "Lower the decoding resolution by a power of two",
+        OFFSET(lowres), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 4, VD },
+    { NULL },
+};
+
+static const AVClass photocd_class = {
+    .class_name = "photocd",
+    .item_name  = av_default_item_name,
+    .option     = options,
+    .version    = LIBAVUTIL_VERSION_INT,
+};
+
+AVCodec ff_photocd_decoder = {
+    .name           = "photocd",
+    .type           = AVMEDIA_TYPE_VIDEO,
+    .id             = AV_CODEC_ID_PHOTOCD,
+    .priv_data_size = sizeof (PhotoCDContext),
+    .priv_class     = &photocd_class,
+    .init           = photocd_decode_init,
+    .close          = photocd_decode_close,
+    .decode         = photocd_decode_frame,
+    .capabilities   = AV_CODEC_CAP_DR1,
+    .long_name      = NULL_IF_CONFIG_SMALL("Kodak Photo CD"),
+};