Re: [FFmpeg-devel] [PATCH]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Carl Eugen Hoyos 
On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:
> On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos  wrote:
> > Hi!
> >
> > Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for
> > me. I can't see how duplicating the code from mov.c could be acceptable.
>
> Personally I found toying with st->priv_data to make it look like
> being in the MOV demuxer not acceptable.

Given that ff_mov_read_stsd_entries() is already used outside of the mov 
demuxer and we generally avoid code duplication and the (very short) 
specification of the structure only mentions it is identical to stsd, I 
consider my patch the more acceptable solution.

> It has the potential for unpredicatble side-effects in the long run.

If this potential exists, it also exists without this patch.

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

Re: [FFmpeg-devel] [PATCH]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Mats Peterson 

On 12/14/2015 12:21 PM, Mats Peterson wrote:

On 12/14/2015 12:16 PM, Mats Peterson wrote:

On 12/14/2015 12:15 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 12:10:48 pm Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 12:03 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes
ticket #5071 for me. I can't see how duplicating the code
from mov.c could be acceptable.


Personally I found toying with st->priv_data to make it look
like being in the MOV demuxer not acceptable.


Given that ff_mov_read_stsd_entries() is already used outside
of the mov demuxer and we generally avoid code duplication and
the (very short) specification of the structure only mentions
it is identical to stsd, I consider my patch the more
acceptable solution.


It has the potential for unpredicatble side-effects in the
long run.


If this potential exists, it also exists without this patch.


Just because its already used elsewhere doesn't make it a good
idea.


So how should we read the stsd atom from non-mov input?

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



Are you ignoring me completely? Not that I'm surprised. I already
explained what I've been doing in my patch. Read it.

Mats



The contents of the stsd atom IS ALREADY IN THE PRIVATE DATA.

Mats



And "how should we read it", well, exactly what I've been doing, by 
using offsets into the private data to get at the various values.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Carl Eugen Hoyos 
On Monday 14 December 2015 12:10:48 pm Hendrik Leppkes wrote:
> On Mon, Dec 14, 2015 at 12:03 PM, Carl Eugen Hoyos wrote:
> > On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:
> >> On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos wrote:
> >> > Hi!
> >> >
> >> > Attached patch based on 3ece3e4c by Martin Storsjö fixes 
> >> > ticket #5071 for me. I can't see how duplicating the code 
> >> > from mov.c could be acceptable.
> >>
> >> Personally I found toying with st->priv_data to make it look 
> >> like being in the MOV demuxer not acceptable.
> >
> > Given that ff_mov_read_stsd_entries() is already used outside 
> > of the mov demuxer and we generally avoid code duplication and 
> > the (very short) specification of the structure only mentions 
> > it is identical to stsd, I consider my patch the more 
> > acceptable solution. 
> >
> >> It has the potential for unpredicatble side-effects in the 
> >> long run. 
> >
> > If this potential exists, it also exists without this patch.
>
> Just because its already used elsewhere doesn't make it a good 
> idea. 

So how should we read the stsd atom from non-mov input?

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

Re: [FFmpeg-devel] [PATCH]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Mats Peterson 

On 12/14/2015 12:03 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos  wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for
me. I can't see how duplicating the code from mov.c could be acceptable.


Personally I found toying with st->priv_data to make it look like
being in the MOV demuxer not acceptable.


Given that ff_mov_read_stsd_entries() is already used outside of the mov
demuxer and we generally avoid code duplication and the (very short)
specification of the structure only mentions it is identical to stsd, I
consider my patch the more acceptable solution.


It has the potential for unpredicatble side-effects in the long run.


If this potential exists, it also exists without this patch.

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



It does not, since in my patch everything happens inside matroskadec.c.

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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Mats Peterson 

On 12/14/2015 12:15 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 12:10:48 pm Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 12:03 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes
ticket #5071 for me. I can't see how duplicating the code
from mov.c could be acceptable.


Personally I found toying with st->priv_data to make it look
like being in the MOV demuxer not acceptable.


Given that ff_mov_read_stsd_entries() is already used outside
of the mov demuxer and we generally avoid code duplication and
the (very short) specification of the structure only mentions
it is identical to stsd, I consider my patch the more
acceptable solution.


It has the potential for unpredicatble side-effects in the
long run.


If this potential exists, it also exists without this patch.


Just because its already used elsewhere doesn't make it a good
idea.


So how should we read the stsd atom from non-mov input?

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



Are you ignoring me completely? Not that I'm surprised. I already 
explained what I've been doing in my patch. Read it.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Mats Peterson 

On 12/14/2015 09:37 AM, Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos  wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for me.
I can't see how duplicating the code from mov.c could be acceptable.



Personally I found toying with st->priv_data to make it look like
being in the MOV demuxer not acceptable.
It has the potential for unpredicatble side-effects in the long run.

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



And it's overkill to parse the stsd atom from the file when you already 
have what you need in the private data.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Mats Peterson 

On 12/14/2015 12:30 PM, Mats Peterson wrote:

On 12/14/2015 12:21 PM, Mats Peterson wrote:

On 12/14/2015 12:16 PM, Mats Peterson wrote:

On 12/14/2015 12:15 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 12:10:48 pm Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 12:03 PM, Carl Eugen Hoyos wrote:

On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:

On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes
ticket #5071 for me. I can't see how duplicating the code
from mov.c could be acceptable.


Personally I found toying with st->priv_data to make it look
like being in the MOV demuxer not acceptable.


Given that ff_mov_read_stsd_entries() is already used outside
of the mov demuxer and we generally avoid code duplication and
the (very short) specification of the structure only mentions
it is identical to stsd, I consider my patch the more
acceptable solution.


It has the potential for unpredicatble side-effects in the
long run.


If this potential exists, it also exists without this patch.


Just because its already used elsewhere doesn't make it a good
idea.


So how should we read the stsd atom from non-mov input?

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



Are you ignoring me completely? Not that I'm surprised. I already
explained what I've been doing in my patch. Read it.

Mats



The contents of the stsd atom IS ALREADY IN THE PRIVATE DATA.

Mats



And "how should we read it", well, exactly what I've been doing, by
using offsets into the private data to get at the various values.

Mats



I bet you haven't even looked at my patch yet, Carl. Do it, and you will 
(hopefully) understand.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Hendrik Leppkes 
On Mon, Dec 14, 2015 at 12:03 PM, Carl Eugen Hoyos  wrote:
> On Monday 14 December 2015 09:37:22 am Hendrik Leppkes wrote:
>> On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos  wrote:
>> > Hi!
>> >
>> > Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for
>> > me. I can't see how duplicating the code from mov.c could be acceptable.
>>
>> Personally I found toying with st->priv_data to make it look like
>> being in the MOV demuxer not acceptable.
>
> Given that ff_mov_read_stsd_entries() is already used outside of the mov
> demuxer and we generally avoid code duplication and the (very short)
> specification of the structure only mentions it is identical to stsd, I
> consider my patch the more acceptable solution.
>
>> It has the potential for unpredicatble side-effects in the long run.
>
> If this potential exists, it also exists without this patch.
>

Just because its already used elsewhere doesn't make it a good idea.

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

Re: [FFmpeg-devel] [PATCH]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-14 Thread Hendrik Leppkes 
On Mon, Dec 14, 2015 at 3:40 AM, Carl Eugen Hoyos  wrote:
> Hi!
>
> Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for me.
> I can't see how duplicating the code from mov.c could be acceptable.
>

Personally I found toying with st->priv_data to make it look like
being in the MOV demuxer not acceptable.
It has the potential for unpredicatble side-effects in the long run.

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

Re: [FFmpeg-devel] [PATCH]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-13 Thread Mats Peterson 

On 12/14/2015 03:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for me.
I can't see how duplicating the code from mov.c could be acceptable.

Please comment, Carl Eugen



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



If you look at the patch I've made, it's not DUPLICATING the code, it 
BORROWS some of code for the loops, but they are modified as well, if 
you look closely, and it uses the PRIVATE DATA instead of calling some 
superfluous stsd atom parsing function that reads from the file. I hope 
Michael Niedermayer understands this better than you do, at least.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-13 Thread Mats Peterson 

On 12/14/2015 03:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071 for me.
I can't see how duplicating the code from mov.c could be acceptable.

Please comment, Carl Eugen



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



You can't? Then why did Hendryk tell me that calling code in another 
demuxer was a bad idea? That's why I changed the whole thing. And it IS 
a bad idea, since that code in mov.c is reading from the FILE, which is 
totally overkill when you already have the private data to work with. 
It's not up to you to decide this, please leave it to someone who knows 
the issue better than you.


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]lavf/matroskadec: Parse stsd atom in mkv with the appropriate mov function

2015-12-13 Thread Mats Peterson 

On 12/14/2015 06:32 AM, Mats Peterson wrote:

On 12/14/2015 03:40 AM, Carl Eugen Hoyos wrote:

Hi!

Attached patch based on 3ece3e4c by Martin Storsjö fixes ticket #5071
for me.
I can't see how duplicating the code from mov.c could be acceptable.

Please comment, Carl Eugen



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



If you look at the patch I've made, it's not DUPLICATING the code, it
BORROWS some of code for the loops, but they are modified as well, if
you look closely, and it uses the PRIVATE DATA instead of calling some
superfluous stsd atom parsing function that reads from the file. I hope
Michael Niedermayer understands this better than you do, at least.

Mats



And even if your patch will be the one to use (against all odds), it is 
still lacking the code that puts the palette in extradata, a la 
V_MS/VFW/FOURCC. Without it, MPlayer won't see the 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