Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-27 Thread Mats Peterson 

On 12/27/2015 10:30 AM, Hendrik Leppkes wrote:

On Sun, Dec 27, 2015 at 4:42 AM, Mats Peterson
 wrote:

On 12/27/2015 03:57 AM, Mats Peterson wrote:


On 12/27/2015 03:03 AM, Michael Niedermayer wrote:


+
+if (!(stsd = av_malloc(70)))
+return AVERROR(ENOMEM);



the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling



Yes, I thought so. I tried to be "a good boy", but that was obviously to
no avail ;)


+int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
+uint32_t *palette);
+



missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...



Regarding doxy documentation, I notice several files in libavformat are
lacking doxy documentation (if what you mean by "doxy documentation" is
a comment beginning with /**). I don't know what to put it in either, at
that. Please help me out.

And regarding two inputs, well, the problem is that matroskadec.c has
the video sample description stored in its in-memory private data, while
mov.c reads the video sample description from the file. I don't want to
mess too much with the logic in mov.c, that's why I provide both a
"memory" and a "file" input. Confusing, yes, slightly, but necessary as
long as you want a common function to be called from both sources. If
anyone else manages to come up with something better WITHOUT BREAKING
IT, no problem. It does take some knowledge about the structure of a
QuickTime video sample description.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



Actually I would prefer that nobody touches what I've been doing, since it
works just fine right now, and it can be easily broken if you start trying
to "improve it". Belive me, I've tried.



And we would prefer if code is actually clean and not a convoluted
mess, and if you don't want us improving it, and you don't want to do
it yourself, then thats too bad.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



It's not a convoluted mess. It's better to have one function that is 
called by two different demuxers than duplicating code. And since it 
works well right now, why change it? We will see what Michael 
Niedermayer says about this. After all, he is the maintainer.


Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-27 Thread Mats Peterson 

On 12/27/2015 10:38 AM, Mats Peterson wrote:

On 12/27/2015 10:30 AM, Hendrik Leppkes wrote:

On Sun, Dec 27, 2015 at 4:42 AM, Mats Peterson
 wrote:

On 12/27/2015 03:57 AM, Mats Peterson wrote:


On 12/27/2015 03:03 AM, Michael Niedermayer wrote:


+
+if (!(stsd = av_malloc(70)))
+return AVERROR(ENOMEM);



the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling



Yes, I thought so. I tried to be "a good boy", but that was
obviously to
no avail ;)


+int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
+uint32_t *palette);
+



missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...



Regarding doxy documentation, I notice several files in libavformat are
lacking doxy documentation (if what you mean by "doxy documentation" is
a comment beginning with /**). I don't know what to put it in
either, at
that. Please help me out.

And regarding two inputs, well, the problem is that matroskadec.c has
the video sample description stored in its in-memory private data,
while
mov.c reads the video sample description from the file. I don't want to
mess too much with the logic in mov.c, that's why I provide both a
"memory" and a "file" input. Confusing, yes, slightly, but necessary as
long as you want a common function to be called from both sources. If
anyone else manages to come up with something better WITHOUT BREAKING
IT, no problem. It does take some knowledge about the structure of a
QuickTime video sample description.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



Actually I would prefer that nobody touches what I've been doing,
since it
works just fine right now, and it can be easily broken if you start
trying
to "improve it". Belive me, I've tried.



And we would prefer if code is actually clean and not a convoluted
mess, and if you don't want us improving it, and you don't want to do
it yourself, then thats too bad.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



It's not a convoluted mess. It's better to have one function that is
called by two different demuxers than duplicating code. And since it
works well right now, why change it? We will see what Michael
Niedermayer says about this. After all, he is the maintainer.

Mats



Read the explanation above thoroughly, and you will hopefully understand 
the problem.


Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-26 Thread Michael Niedermayer 
On Wed, Dec 23, 2015 at 03:37:18PM +0100, Mats Peterson wrote:
> I mistakenly used 'extradata' rather than 'st->codec->extradata',
> naturally leading to a segfault. Here's an updated patch.
> 
> An explanation of the patch follows:
> 
> Palettized QuickTime video in Matroska has hitherto not been
> recognized whatsoever, and the "palette" used has been completely
> random.
> 
> The patch for matroskadec.c fixes this issue by adding a palette
> side data packet in matroska_deliver_packet(), much in the same way
> as it's done in mov.c.
> 
> The change to mov.c consists mainly of moving the palette handling
> from the mov_parse_stsd_video() function to a new ff_get_qtpalette()
> function in the new file qtpalette.c, which is shared by both
> matroskadec.c and mov.c.
> 
> In matroskadec.c, I'm also putting the palette in 'extradata', like
> it's done for V_MS/VFW/FOURCC; this is a requirement in order for
> MPlayer to recognize the palette. And why is this, you may wonder.
> Well, it's because for some mysterious reason, MPlayer adds ANOTHER
> palette side data packet after the one added in matroskadec.c. It
> uses whatever is in extradata as the palette when adding this
> packet.
> 
> Mats
> 
> -- 
> Mats Peterson
> http://matsp888.no-ip.org/~mats/

>  Makefile  |1 
>  matroskadec.c |   30 +-
>  mov.c |   92 +--
>  qtpalette.c   |  123 
> ++
>  qtpalette.h   |6 ++
>  5 files changed, 178 insertions(+), 74 deletions(-)
> 46ebbf197c0580b7588cb4550373a05489454cae  
> 0001-lavf-palettized-QuickTime-video-in-Matroska.patch
> From 91376a9e4e0cb4a7522be565e248616c4803c0ad Mon Sep 17 00:00:00 2001
> From: Mats Peterson 
> Date: Wed, 23 Dec 2015 15:30:15 +0100
> Subject: [PATCH v6] lavf: palettized QuickTime video in Matroska
[...]
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 06e80c8..5b0c5af 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c

> @@ -1751,13 +1751,20 @@ static int mov_codec_id(AVStream *st, uint32_t format)
>  return id;
>  }
>  
> -static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
> +static int mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>   AVStream *st, MOVStreamContext *sc)
>  {
> +uint8_t *stsd;
>  uint8_t codec_name[32];
> -unsigned int color_depth, len, j;
> -int color_greyscale;
> -int color_table_id;
> +int64_t pos;
> +unsigned int len;
> +
> +if (!(stsd = av_malloc(70)))
> +return AVERROR(ENOMEM);

the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling

[...]

> @@ -310,4 +311,7 @@ static const uint8_t ff_qt_default_palette_256[256 * 3] = 
> {
>/* 255, 0xFF */  0x00, 0x00, 0x00
>  };
>  
> +int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
> +uint32_t *palette);
> +

missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-26 Thread Mats Peterson 

On 12/27/2015 03:03 AM, Michael Niedermayer wrote:

+
+if (!(stsd = av_malloc(70)))
+return AVERROR(ENOMEM);


the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling


Yes, I thought so. I tried to be "a good boy", but that was obviously to 
no avail ;)



+int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
+uint32_t *palette);
+


missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...


Regarding doxy documentation, I notice several files in libavformat are 
lacking doxy documentation (if what you mean by "doxy documentation" is 
a comment beginning with /**). I don't know what to put it in either, at 
that. Please help me out.


And regarding two inputs, well, the problem is that matroskadec.c has 
the video sample description stored in its in-memory private data, while 
mov.c reads the video sample description from the file. I don't want to 
mess too much with the logic in mov.c, that's why I provide both a 
"memory" and a "file" input. Confusing, yes, slightly, but necessary as 
long as you want a common function to be called from both sources. If 
anyone else manages to come up with something better WITHOUT BREAKING 
IT, no problem. It does take some knowledge about the structure of a 
QuickTime video sample description.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-26 Thread Mats Peterson 

On 12/27/2015 03:57 AM, Mats Peterson wrote:

On 12/27/2015 03:03 AM, Michael Niedermayer wrote:

+
+if (!(stsd = av_malloc(70)))
+return AVERROR(ENOMEM);


the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling


Yes, I thought so. I tried to be "a good boy", but that was obviously to
no avail ;)


+int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
+uint32_t *palette);
+


missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...


Regarding doxy documentation, I notice several files in libavformat are
lacking doxy documentation (if what you mean by "doxy documentation" is
a comment beginning with /**). I don't know what to put it in either, at
that. Please help me out.

And regarding two inputs, well, the problem is that matroskadec.c has
the video sample description stored in its in-memory private data, while
mov.c reads the video sample description from the file. I don't want to
mess too much with the logic in mov.c, that's why I provide both a
"memory" and a "file" input. Confusing, yes, slightly, but necessary as
long as you want a common function to be called from both sources. If
anyone else manages to come up with something better WITHOUT BREAKING
IT, no problem. It does take some knowledge about the structure of a
QuickTime video sample description.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Actually I would prefer that nobody touches what I've been doing, since 
it works just fine right now, and it can be easily broken if you start 
trying to "improve it". Belive me, I've tried.


Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-26 Thread Mats Peterson 

On 12/27/2015 04:42 AM, Mats Peterson wrote:

On 12/27/2015 03:57 AM, Mats Peterson wrote:

On 12/27/2015 03:03 AM, Michael Niedermayer wrote:

+
+if (!(stsd = av_malloc(70)))
+return AVERROR(ENOMEM);


the malloc is unneeded, an array on the stack could be used (its just
a fixed 70 bytes)
this would also simplify the error handling


Yes, I thought so. I tried to be "a good boy", but that was obviously to
no avail ;)


+int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
+uint32_t *palette);
+


missing doxy documentation, missing "const" for unchanged arrays
also why does this need a "byte" array and a AVIOContext as input
arguments ?
iam asking as this looks a bit confusing with 2 inputs ...


Regarding doxy documentation, I notice several files in libavformat are
lacking doxy documentation (if what you mean by "doxy documentation" is
a comment beginning with /**). I don't know what to put it in either, at
that. Please help me out.

And regarding two inputs, well, the problem is that matroskadec.c has
the video sample description stored in its in-memory private data, while
mov.c reads the video sample description from the file. I don't want to
mess too much with the logic in mov.c, that's why I provide both a
"memory" and a "file" input. Confusing, yes, slightly, but necessary as
long as you want a common function to be called from both sources. If
anyone else manages to come up with something better WITHOUT BREAKING
IT, no problem. It does take some knowledge about the structure of a
QuickTime video sample description.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Actually I would prefer that nobody touches what I've been doing, since
it works just fine right now, and it can be easily broken if you start
trying to "improve it". Belive me, I've tried.

Mats



Michael, will this pass as doxy documentation for ff_get_qtpalette()?

/**
 * Retrieve the palette (or "color table" in QuickTime terms), either
 * from the video sample description, or from the default Macintosh
 * palette.
 */


Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v6] lavf: palettized QuickTime video in Matroska

2015-12-23 Thread Mats Peterson 

On 12/23/2015 03:37 PM, Mats Peterson wrote:

I mistakenly used 'extradata' rather than 'st->codec->extradata',
naturally leading to a segfault. Here's an updated patch.

An explanation of the patch follows:

Palettized QuickTime video in Matroska has hitherto not been recognized
whatsoever, and the "palette" used has been completely random.

The patch for matroskadec.c fixes this issue by adding a palette side
data packet in matroska_deliver_packet(), much in the same way as it's
done in mov.c.

The change to mov.c consists mainly of moving the palette handling from
the mov_parse_stsd_video() function to a new ff_get_qtpalette() function
in the new file qtpalette.c, which is shared by both matroskadec.c and
mov.c.

In matroskadec.c, I'm also putting the palette in 'extradata', like it's
done for V_MS/VFW/FOURCC; this is a requirement in order for MPlayer to
recognize the palette. And why is this, you may wonder. Well, it's
because for some mysterious reason, MPlayer adds ANOTHER palette side
data packet after the one added in matroskadec.c. It uses whatever is in
extradata as the palette when adding this packet.

Mats



___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



Samples: https://drive.google.com/open?id=0B3_pEBoLs0faWElmM2FnLTZYNlk

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel