[RFC] WC3 decoder without AVPaletteControl
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
Bugs are probably still to be found, but FATE test now matches original output, I forgot a round() in gamma_corr()
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
A minor update. It only fixes the gamma correction and provides a fixed point integer-implementation, which might help explain some of its weirdness like clipping to 253.
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
A minor update. It only fixes the gamma correction and provides a fixed point integer-implementation, which might help explain some of its weirdness like clipping to 253.
Mike is ok with, any comments on the av_grow_packet/av_append_packet? I don't like it much, but without such helper functions that buffer management stuff is very tedious (and I am really confident that at least idcin.c needs the same stuff). I'm also happy of any suggestion that makes this kind of thing (merging packets etc.) easier that does not require seeking. The "use case" is this: there is a tag, then a size field and then the data. Multiple of these tag-size-data things should end up in one AVPacket. The least horrible solution I found was: read tag and size, seek back to tag and av_append_packet the whole tag, size and data.
On Tue, Apr 14, 2009 at 05:22:55PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
A minor update. It only fixes the gamma correction and provides a fixed point integer-implementation, which might help explain some of its weirdness like clipping to 253.
Mike is ok with, any comments on the av_grow_packet/av_append_packet?
I was thinking about avoiding the need for av_grow_packet and instead use av_new_packet + memcpy, but there currently isn't a reliable way to copy everything necessary like pos, pts etc. to the new packet (doing it manually in the function will break as soon as a new field is added and someone forgets to update that function). If there are no comments I threaten to apply "somewhen".
On Wed, Apr 15, 2009 at 12:34:26PM +0200, Reimar D?ffinger wrote:
On Tue, Apr 14, 2009 at 05:22:55PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
A minor update. It only fixes the gamma correction and provides a fixed point integer-implementation, which might help explain some of its weirdness like clipping to 253.
Mike is ok with, any comments on the av_grow_packet/av_append_packet?
I was thinking about avoiding the need for av_grow_packet and instead use av_new_packet + memcpy, but there currently isn't a reliable way to copy everything necessary like pos, pts etc. to the new packet (doing it manually in the function will break as soon as a new field is added and someone forgets to update that function). If there are no comments I threaten to apply "somewhen".
please wait 3 days or so, id like to fix avi and see what that will need and if that method could also be used then everywhere i never felt the need for a grow or append packet function when thinking about palettes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On Wed, Apr 15, 2009 at 02:06:55PM +0200, Michael Niedermayer wrote:
i never felt the need for a grow or append packet function when thinking about palettes
Well, the problem is that at least for some formats the palettes are stored apart from the frame data, so at least you have to store them until you have the video data. Now add to that that some formats allow an "arbitrary" number of palettes or other "opcodes" that operate on the palette that you all need to merge into one AVPacket, and you must set the position to that of the first part so you can merge them all together, if possible handling read/malloc errors at least somewhat sensibly and consistently it seemed like the best approach to do it. If you had a look at the CDXL demuxer I think the issue is similar: you have a header which contains the size of video and palette packets and some other information the decoder needs anyway. You can either manually write the information the decoder needs into a manually allocated AVPacket, hoping someone will notice the missing integer overflow check, or you just read the header via av_get_packet, extract the sizes, and use av_append_packet to have a full video packet with all the info necessary. AVI doesn't really have _that_ issue, it always has exactly one fully coded palette AFAICT.
On Wed, Apr 15, 2009 at 02:40:43PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 02:06:55PM +0200, Michael Niedermayer wrote:
i never felt the need for a grow or append packet function when thinking about palettes
Well, the problem is that at least for some formats the palettes are stored apart from the frame data, so at least you have to store them until you have the video data. Now add to that that some formats allow an "arbitrary" number of palettes or other "opcodes" that operate on the palette that you all need to merge into one AVPacket, and you must set the position to that of the first part so you can merge them all together, if possible handling read/malloc errors at least somewhat sensibly and consistently it seemed like the best approach to do it.
If you had a look at the CDXL demuxer I think the issue is similar: you have a header which contains the size of video and palette packets and some other information the decoder needs anyway. You can either manually write the information the decoder needs into a manually allocated AVPacket, hoping someone will notice the missing integer overflow check, or you just read the header via av_get_packet, extract the sizes, and use av_append_packet to have a full video packet with all the info necessary.
realloc() is not free, it needs time and i dont like doing 1 realloc call per packet if it can be avoided. and someone working with demuxers or decoders needs to understand the issues with integer overflows IMHO [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
On Wed, Apr 15, 2009 at 03:26:28PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 02:40:43PM +0200, Reimar D?ffinger wrote:
If you had a look at the CDXL demuxer I think the issue is similar: you have a header which contains the size of video and palette packets and some other information the decoder needs anyway. You can either manually write the information the decoder needs into a manually allocated AVPacket, hoping someone will notice the missing integer overflow check, or you just read the header via av_get_packet, extract the sizes, and use av_append_packet to have a full video packet with all the info necessary.
realloc() is not free, it needs time and i dont like doing 1 realloc call per packet if it can be avoided.
We are talking about packets that will often be 4 - 32 bytes before, it may not be free, but AV_WL32 or whatever isn't free either.
and someone working with demuxers or decoders needs to understand the issues with integer overflows IMHO
There's a difference between understanding and remembering each time you have to write yet another ten lines of completely boring boilerplate code or it simply gets more complex (which I am sure you know, unless you intend to claim r18405 r18393 r18388 all were necessary only because someone did not understand the issues with buffer overflows...).
On Wed, Apr 15, 2009 at 03:50:56PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 03:26:28PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 02:40:43PM +0200, Reimar D?ffinger wrote:
If you had a look at the CDXL demuxer I think the issue is similar: you have a header which contains the size of video and palette packets and some other information the decoder needs anyway. You can either manually write the information the decoder needs into a manually allocated AVPacket, hoping someone will notice the missing integer overflow check, or you just read the header via av_get_packet, extract the sizes, and use av_append_packet to have a full video packet with all the info necessary.
realloc() is not free, it needs time and i dont like doing 1 realloc call per packet if it can be avoided.
We are talking about packets that will often be 4 - 32 bytes before, it may not be free, but AV_WL32 or whatever isn't free either.
And what is the relative speedof AV_WL32() and realloc(4) ? besides, it seems your functions may be usefull for avi too [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
On Wed, Apr 15, 2009 at 04:21:01PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:50:56PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 03:26:28PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 02:40:43PM +0200, Reimar D?ffinger wrote:
If you had a look at the CDXL demuxer I think the issue is similar: you have a header which contains the size of video and palette packets and some other information the decoder needs anyway. You can either manually write the information the decoder needs into a manually allocated AVPacket, hoping someone will notice the missing integer overflow check, or you just read the header via av_get_packet, extract the sizes, and use av_append_packet to have a full video packet with all the info necessary.
realloc() is not free, it needs time and i dont like doing 1 realloc call per packet if it can be avoided.
We are talking about packets that will often be 4 - 32 bytes before, it may not be free, but AV_WL32 or whatever isn't free either.
And what is the relative speedof AV_WL32() and realloc(4) ?
It depends? :-) But seriously, it's not like I don't see your point, I was thinking about e.g. adding functions that would just build the packet data into a buffer created via e.g. av_fast_realloc, but then you'd still need extra code to track the right value for e.g. pkt->pos, and you'd still need a function that creates a AVPacket from a buffer and in the end went with "forget it, those formats were for 286 and 386 PCs making this convenient to use _is_ more important than speed". I know you are more reluctant to just ignore the speed implications which was another reason to think a bit longer about it, but so far those functions still seem like the best speed vs. usability compromise for those low complexity, "fringe" formats.
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
On Sun, Apr 12, 2009 at 12:19:07AM +0200, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
A minor update. It only fixes the gamma correction and provides a fixed point integer-implementation, which might help explain some of its weirdness like clipping to 253.
[...]
Index: libavcodec/avcodec.h =================================================================== --- libavcodec/avcodec.h (revision 18467) +++ libavcodec/avcodec.h (working copy) @@ -2666,6 +2666,14 @@ void av_shrink_packet(AVPacket *pkt, int size);
/** + * Increase packet size, correctly zeroing padding + * + * @param pkt packet + * @param grow_by number of bytes by which to increase the size of the packet + */ +int av_grow_packet(AVPacket *pkt, int grow_by); + +/** * @warning This is a hack - the packet memory allocation stuff is broken. The * packet is allocated if it was not really allocated. */ Index: libavcodec/avpacket.c =================================================================== --- libavcodec/avpacket.c (revision 18467) +++ libavcodec/avpacket.c (working copy) @@ -69,6 +69,24 @@ memset(pkt->data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE); }
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr;
+ if (!grow_by) + return 0;
for what is that good for?
+ if (!pkt->size) + return av_new_packet(pkt, grow_by);
+ if (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) + return -1;
could you write this in a way that is free of overflows please ;) [...]
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
On Wed, Apr 15, 2009 at 04:17:24PM +0200, Michael Niedermayer wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr;
+ if (!grow_by) + return 0;
for what is that good for?
Avoiding a pointless realloc in case someone does something stupid like av_grow_packet(pkt, 0);
+ if (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) + return -1;
could you write this in a way that is free of overflows please ;)
Hm. Bad "documentation", I hadn't decided what grow_by < 0 is supposed to mean or if I should just make it unsigned (the latter I admit would not help here). So
if (grow_by < 0 || pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) good enough? I also assumed that pkt->size + FF_INPUT_BUFFER_PADDING_SIZE would not overflow since the allocated buffer must already be that big, if you dislike that.
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
On Wed, Apr 15, 2009 at 05:29:14PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 04:17:24PM +0200, Michael Niedermayer wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr;
+ if (!grow_by) + return 0;
for what is that good for?
Avoiding a pointless realloc in case someone does something stupid like av_grow_packet(pkt, 0);
people should not do such stupid things
+ if (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) + return -1;
could you write this in a way that is free of overflows please ;)
Hm. Bad "documentation", I hadn't decided what grow_by < 0 is supposed to mean or if I should just make it unsigned (the latter I admit would not help here). So
if (grow_by < 0 || pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) good enough? I also assumed that pkt->size + FF_INPUT_BUFFER_PADDING_SIZE would not overflow since the allocated buffer must already be that big, if you dislike that.
maybe (unsigned)grow_by > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE || pkt->size > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE - grow_by ?
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
well, fine leave that then [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
I'll try once more to fix the palette issues... On Wed, Apr 15, 2009 at 08:47:24PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 05:29:14PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 04:17:24PM +0200, Michael Niedermayer wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr;
+ if (!grow_by) + return 0;
for what is that good for?
Avoiding a pointless realloc in case someone does something stupid like av_grow_packet(pkt, 0);
people should not do such stupid things
+ if (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) + return -1;
could you write this in a way that is free of overflows please ;)
Hm. Bad "documentation", I hadn't decided what grow_by < 0 is supposed to mean or if I should just make it unsigned (the latter I admit would not help here). So
if (grow_by < 0 || pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) good enough? I also assumed that pkt->size + FF_INPUT_BUFFER_PADDING_SIZE would not overflow since the allocated buffer must already be that big, if you dislike that.
maybe
(unsigned)grow_by > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE || pkt->size > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE - grow_by
?
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
well, fine leave that then
One more try...
On Thu, Nov 18, 2010 at 10:18:08PM +0100, Reimar D?ffinger wrote:
I'll try once more to fix the palette issues...
On Wed, Apr 15, 2009 at 08:47:24PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 05:29:14PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 04:17:24PM +0200, Michael Niedermayer wrote:
On Sun, Apr 12, 2009 at 01:26:33PM +0200, Reimar D?ffinger wrote:
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr;
+ if (!grow_by) + return 0;
for what is that good for?
Avoiding a pointless realloc in case someone does something stupid like av_grow_packet(pkt, 0);
people should not do such stupid things
+ if (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) + return -1;
could you write this in a way that is free of overflows please ;)
Hm. Bad "documentation", I hadn't decided what grow_by < 0 is supposed to mean or if I should just make it unsigned (the latter I admit would not help here). So
if (grow_by < 0 || pkt->size + FF_INPUT_BUFFER_PADDING_SIZE > INT_MAX - grow_by) good enough? I also assumed that pkt->size + FF_INPUT_BUFFER_PADDING_SIZE would not overflow since the allocated buffer must already be that big, if you dislike that.
maybe
(unsigned)grow_by > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE || pkt->size > INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE - grow_by
?
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
well, fine leave that then
One more try...
is it intended to change the table to dynmically generated in this patch? is the code even smaller than the 256 bytes?
libavcodec/avcodec.h | 8 +++ libavcodec/avpacket.c | 20 +++++++- libavcodec/xan.c | 117 ++++++++++++++++++++++++++++++++++++++++++++--- libavformat/avformat.h | 11 ++++ libavformat/utils.c | 14 +++++ libavformat/wc3movie.c | 120 +++++++------------------------------------------ 6 files changed, 180 insertions(+), 110 deletions(-) 9a9e24da63ec55a4343383c70e733205f50f2f9c wc3_pal.diff Index: libavcodec/avcodec.h =================================================================== --- libavcodec/avcodec.h (revision 25765) +++ libavcodec/avcodec.h (working copy) @@ -3027,6 +3027,14 @@ void av_shrink_packet(AVPacket *pkt, int size);
/** + * Increase packet size, correctly zeroing padding + * + * @param pkt packet + * @param grow_by number of bytes by which to increase the size of the packet + */ +int av_grow_packet(AVPacket *pkt, int grow_by);
maybe this should be size_t [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
On Sat, Nov 20, 2010 at 05:16:49AM +0100, Michael Niedermayer wrote:
On Thu, Nov 18, 2010 at 10:18:08PM +0100, Reimar D?ffinger wrote:
Index: libavformat/utils.c =================================================================== --- libavformat/utils.c (revision 18467) +++ libavformat/utils.c (working copy) @@ -277,6 +277,20 @@ return ret; }
+int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) +{ + int ret; + int old_size; + if (!pkt->size) + return av_get_packet(s, pkt, size); + old_size = pkt->size; + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
well, fine leave that then
One more try...
is it intended to change the table to dynmically generated in this patch? is the code even smaller than the 256 bytes?
No, this is actually only meant as documentation. I'll add that in a separate patch and keep the table by default.
9a9e24da63ec55a4343383c70e733205f50f2f9c wc3_pal.diff Index: libavcodec/avcodec.h =================================================================== --- libavcodec/avcodec.h (revision 25765) +++ libavcodec/avcodec.h (working copy) @@ -3027,6 +3027,14 @@ void av_shrink_packet(AVPacket *pkt, int size);
/** + * Increase packet size, correctly zeroing padding + * + * @param pkt packet + * @param grow_by number of bytes by which to increase the size of the packet + */ +int av_grow_packet(AVPacket *pkt, int grow_by);
maybe this should be size_t
Well, av_new_packet uses int as well, so that would be inconsistent IMO. And I still don't like the idea of code that behaves differently on different architectures. Oh, and in case someone wants to test it, this file: http://samples.mplayerhq.hu/game-formats/wc3-mve/wc3-successful-mission.mve after the patch plays correctly in ffplay, before it broke for a short time during a palette change. Update patch attached, it also contains a change to SHOT_TAG handling so that an invalid value there does not cause the palette plane to be uninitialized.
On Sat, Nov 20, 2010 at 11:01:29AM +0100, Reimar D?ffinger wrote:
On Sat, Nov 20, 2010 at 05:16:49AM +0100, Michael Niedermayer wrote:
On Thu, Nov 18, 2010 at 10:18:08PM +0100, Reimar D?ffinger wrote:
> Index: libavformat/utils.c > =================================================================== > --- libavformat/utils.c (revision 18467) > +++ libavformat/utils.c (working copy) > @@ -277,6 +277,20 @@ > return ret; > } > > +int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size) > +{ > + int ret; > + int old_size; > + if (!pkt->size) > + return av_get_packet(s, pkt, size); > + old_size = pkt->size; > + ret = av_grow_packet(pkt, size);
isnt av_grow_packet already checking for pkt->size==0 ?
Yes, but it only allocates a new packet then. av_get_packet also sets pkt->pos. Sure, maybe a bit overkill for a single assignment, just thought it might be more future-proof.
well, fine leave that then
One more try...
is it intended to change the table to dynmically generated in this patch? is the code even smaller than the 256 bytes?
No, this is actually only meant as documentation. I'll add that in a separate patch and keep the table by default.
9a9e24da63ec55a4343383c70e733205f50f2f9c wc3_pal.diff Index: libavcodec/avcodec.h =================================================================== --- libavcodec/avcodec.h (revision 25765) +++ libavcodec/avcodec.h (working copy) @@ -3027,6 +3027,14 @@ void av_shrink_packet(AVPacket *pkt, int size);
/** + * Increase packet size, correctly zeroing padding + * + * @param pkt packet + * @param grow_by number of bytes by which to increase the size of the packet + */ +int av_grow_packet(AVPacket *pkt, int grow_by);
maybe this should be size_t
Well, av_new_packet uses int as well, so that would be inconsistent IMO. And I still don't like the idea of code that behaves differently on different architectures. Oh, and in case someone wants to test it, this file: http://samples.mplayerhq.hu/game-formats/wc3-mve/wc3-successful-mission.mve after the patch plays correctly in ffplay, before it broke for a short time during a palette change. Update patch attached, it also contains a change to SHOT_TAG handling so that an invalid value there does not cause the palette plane to be uninitialized.
libavcodec/avcodec.h | 8 +++ libavcodec/avpacket.c | 20 ++++++++ libavcodec/xan.c | 106 +++++++++++++++++++++++++++++++++++++++++--- libavformat/avformat.h | 11 ++++ libavformat/utils.c | 14 +++++ libavformat/wc3movie.c | 116 +++++++------------------------------------------ 6 files changed, 169 insertions(+), 106 deletions(-) d593eae626bf104fa2bb0d7ff16eb0b9ae22ecd4 wc3_pal.diff Index: libavcodec/avcodec.h =================================================================== --- libavcodec/avcodec.h (revision 25774) +++ libavcodec/avcodec.h (working copy) @@ -3027,6 +3027,14 @@ void av_shrink_packet(AVPacket *pkt, int size);
/** + * Increase packet size, correctly zeroing padding + * + * @param pkt packet + * @param grow_by number of bytes by which to increase the size of the packet + */ +int av_grow_packet(AVPacket *pkt, int grow_by); + +/** * @warning This is a hack - the packet memory allocation stuff is broken. The * packet is allocated if it was not really allocated. */
Index: libavcodec/avpacket.c =================================================================== --- libavcodec/avpacket.c (revision 25774) +++ libavcodec/avpacket.c (working copy) @@ -20,6 +20,7 @@ */
#include "avcodec.h" +#include "libavutil/avassert.h"
void av_destruct_packet_nofree(AVPacket *pkt)
@@ -48,7 +49,7 @@ int av_new_packet(AVPacket *pkt, int size) { uint8_t *data= NULL; - if((unsigned)size < (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE) + if((unsigned)size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE) data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); if (data){ memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
i think av_malloc alraedy protects us against this, that said if you prefer then this change is surely ok as a seperate commit
@@ -71,6 +72,23 @@ memset(pkt->data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE); }
+int av_grow_packet(AVPacket *pkt, int grow_by) +{ + void *new_ptr; + av_assert0((unsigned)pkt->size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE); + if (!pkt->size) + return av_new_packet(pkt, grow_by); + if ((unsigned)grow_by > INT_MAX - (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE)) + return -1; + new_ptr = av_realloc(pkt->data, pkt->size + grow_by + FF_INPUT_BUFFER_PADDING_SIZE); + if (!new_ptr) + return AVERROR(ENOMEM); + pkt->data = new_ptr; + pkt->size += grow_by; + memset(pkt->data + pkt->size, 0, FF_INPUT_BUFFER_PADDING_SIZE); + return 0; +} + int av_dup_packet(AVPacket *pkt) { if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == NULL)) && pkt->data) {
that should also be seperate from xan/wc* ideally
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 25774) +++ libavformat/avformat.h (working copy) @@ -245,6 +245,17 @@ int av_get_packet(ByteIOContext *s, AVPacket *pkt, int size);
+/** + * Reads data and appends it to the current content of the AVPacket. + * If pkt->size is 0 it behaves like av_get_packet.
This should mention that its inefficient and better to allocate a correctly sized packet [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On Sat, Nov 20, 2010 at 05:14:25PM +0100, Michael Niedermayer wrote:
@@ -48,7 +49,7 @@ int av_new_packet(AVPacket *pkt, int size) { uint8_t *data= NULL; - if((unsigned)size < (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE) + if((unsigned)size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE) data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); if (data){ memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
i think av_malloc alraedy protects us against this, that said if you prefer then this change is surely ok as a seperate commit
I didn't like the idea of relying on it, also because av_malloc at least currently uses a hardcoded "16" (which I suspect is for the "memalign hack" version) and not FF_INPUT_BUFFER_PADDING_SIZE.
int av_dup_packet(AVPacket *pkt) { if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == NULL)) && pkt->data) {
that should also be seperate from xan/wc* ideally
Yes, I will of course commit the separately, first the extra av_new_packet check, then the grow_packet, then the append function and last the wc3 change was the plan.
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 25774) +++ libavformat/avformat.h (working copy) @@ -245,6 +245,17 @@ int av_get_packet(ByteIOContext *s, AVPacket *pkt, int size);
+/** + * Reads data and appends it to the current content of the AVPacket. + * If pkt->size is 0 it behaves like av_get_packet.
This should mention that its inefficient and better to allocate a correctly sized packet
Done. More comments?
On Sat, Nov 20, 2010 at 06:38:56PM +0100, Reimar D?ffinger wrote:
On Sat, Nov 20, 2010 at 05:14:25PM +0100, Michael Niedermayer wrote:
@@ -48,7 +49,7 @@ int av_new_packet(AVPacket *pkt, int size) { uint8_t *data= NULL; - if((unsigned)size < (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE) + if((unsigned)size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE) data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); if (data){ memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
i think av_malloc alraedy protects us against this, that said if you prefer then this change is surely ok as a seperate commit
I didn't like the idea of relying on it, also because av_malloc at least currently uses a hardcoded "16" (which I suspect is for the "memalign hack" version) and not FF_INPUT_BUFFER_PADDING_SIZE.
int av_dup_packet(AVPacket *pkt) { if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == NULL)) && pkt->data) {
that should also be seperate from xan/wc* ideally
Yes, I will of course commit the separately, first the extra av_new_packet check, then the grow_packet, then the append function and last the wc3 change was the plan.
Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 25774) +++ libavformat/avformat.h (working copy) @@ -245,6 +245,17 @@ int av_get_packet(ByteIOContext *s, AVPacket *pkt, int size);
+/** + * Reads data and appends it to the current content of the AVPacket. + * If pkt->size is 0 it behaves like av_get_packet.
This should mention that its inefficient and better to allocate a correctly sized packet
Done. More comments?
no further comments from me [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
On Sat, Nov 20, 2010 at 10:06:19PM +0100, Michael Niedermayer wrote:
On Sat, Nov 20, 2010 at 06:38:56PM +0100, Reimar D?ffinger wrote:
On Sat, Nov 20, 2010 at 05:14:25PM +0100, Michael Niedermayer wrote:
@@ -48,7 +49,7 @@ int av_new_packet(AVPacket *pkt, int size) { uint8_t *data= NULL; - if((unsigned)size < (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE) + if((unsigned)size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE) data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); if (data){ memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
i think av_malloc alraedy protects us against this, that said if you prefer then this change is surely ok as a seperate commit
I didn't like the idea of relying on it, also because av_malloc at least currently uses a hardcoded "16" (which I suspect is for the "memalign hack" version) and not FF_INPUT_BUFFER_PADDING_SIZE.
Sorry, I somehow read the code wrongly. I don't think there's a point in that change so I did not apply it.
Done. More comments?
no further comments from me
Applied, also bumping minor versions.
Hi, On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed. Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry. second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already. third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry.
Please enumerate the formats. State exactly what the problem is. Doesn't avi use bmp header ? If so palette -> extradata.
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already.
I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet. If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively
And modifying demuxer require updating every decoder. AVPacket has a useless ->priv flag already it seems. Adding a field is _not_ a problem. There is still the possibility to put in in AVFormatContext. [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 10:51:22AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry.
Please enumerate the formats. State exactly what the problem is.
i think i did
Doesn't avi use bmp header ? If so palette -> extradata.
if you dont belive me you can read the source, look at sample files (like CIMOVI01.avi) or the avi spec
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already.
I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet.
-vcodec copy will not cause problems if muxers and demuxers implement the API correctly, this is true for any not to crappy API.
If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively
And modifying demuxer require updating every decoder.
so would they if it was in AVPacket->palette
AVPacket has a useless ->priv flag already it seems. Adding a field is _not_ a problem.
There is still the possibility to put in in AVFormatContext.
no that is not, we try to fix the race condition that is caused by seperating the palette data from packets [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
On 4/15/2009 11:35 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:51:22AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote: palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry. Please enumerate the formats. State exactly what the problem is.
i think i did
Doesn't avi use bmp header ? If so palette -> extradata.
if you dont belive me you can read the source, look at sample files (like CIMOVI01.avi) or the avi spec
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already. I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet.
-vcodec copy will not cause problems if muxers and demuxers implement the API correctly, this is true for any not to crappy API.
Parsing packet data is annoying in any case and IMHO it will only produces clutter like the AVMedatata language crap, code has proven it.
If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
Well it will work for sure. It's certainly not efficient.
third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively And modifying demuxer require updating every decoder.
so would they if it was in AVPacket->palette
Well, it seems that storing palette in AVFormatContext and let the application updating AVCodecContext won't need decoders to be changed.
AVPacket has a useless ->priv flag already it seems. Adding a field is _not_ a problem.
There is still the possibility to put in in AVFormatContext.
no that is not, we try to fix the race condition that is caused by seperating the palette data from packets
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_. Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ? -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 11:49:54AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:35 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:51:22AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
Hello, attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote: palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry. Please enumerate the formats. State exactly what the problem is.
i think i did
Doesn't avi use bmp header ? If so palette -> extradata.
if you dont belive me you can read the source, look at sample files (like CIMOVI01.avi) or the avi spec
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already. I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet.
-vcodec copy will not cause problems if muxers and demuxers implement the API correctly, this is true for any not to crappy API.
Parsing packet data is annoying in any case and IMHO it will only
well the palette chunks need to be parsed as they arent complete palettes you cant avoid that
produces clutter like the AVMedatata language crap, code has proven it.
i see no problem in AVMedatata
If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
Well it will work for sure.
how exactly would the libvaformat mov demuxer pass these palettes to the decoder?
It's certainly not efficient.
yes, 3600*30*(256*4 + stsd size) vs 3600*30*16 that is 105mb wasted space per hour and id guess also 105mb of ram needed for demuxing & muxing, iam certain that will fail on some embeded hardware, even if just by "out of memory"
third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively And modifying demuxer require updating every decoder.
so would they if it was in AVPacket->palette
Well, it seems that storing palette in AVFormatContext and let the application updating AVCodecContext won't need decoders to be changed.
no, it just needs applications to have their demuxer-decoder interface rewritten to pass frames + palettes Even for us (ffplay) that would not be fun. Its probably the worst of all suggestions ...
AVPacket has a useless ->priv flag already it seems. Adding a field is _not_ a problem.
There is still the possibility to put in in AVFormatContext.
no that is not, we try to fix the race condition that is caused by seperating the palette data from packets
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
On Wed, Apr 15, 2009 at 09:01:38PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 11:49:54AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:35 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:51:22AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
Hi,
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote: > Hello, > attached patch implements this. > It is a bit ugly because it adds a lot of seeking to the demuxer, but I > think for such a fringe format there is no point in extra work for avoiding it. > The patch also adds two helper functions, one to increase the size of a > packet and one that appends data read from a file to a possibly already > filled packet. > I expect that there will be more that a few bugs still, e.g. while the > video looks visually ok in ffplay the FATE regressions do not match at > all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote: palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry. Please enumerate the formats. State exactly what the problem is.
i think i did
Doesn't avi use bmp header ? If so palette -> extradata.
if you dont belive me you can read the source, look at sample files (like CIMOVI01.avi) or the avi spec
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already. I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet.
-vcodec copy will not cause problems if muxers and demuxers implement the API correctly, this is true for any not to crappy API.
Parsing packet data is annoying in any case and IMHO it will only
well the palette chunks need to be parsed as they arent complete palettes you cant avoid that
Yes you can. If you don't append the palette to packet data you don't need to parse the packet.
produces clutter like the AVMedatata language crap, code has proven it.
i see no problem in AVMedatata
Are you blind ? ;)
If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
Well it will work for sure.
how exactly would the libvaformat mov demuxer pass these palettes to the decoder?
Using the same way it does currently ?
[...]
third its another field that bloats small AVPackets, another field that must be handled explicitlyin every user application that does not use AVPackets natively And modifying demuxer require updating every decoder.
so would they if it was in AVPacket->palette
Well, it seems that storing palette in AVFormatContext and let the application updating AVCodecContext won't need decoders to be changed.
no, it just needs applications to have their demuxer-decoder interface rewritten to pass frames + palettes Even for us (ffplay) that would not be fun. Its probably the worst of all suggestions ...
[...]
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much
I don't forget, application will update AVCodecContext palette accordingly, of course. This does not remove palette support at all, and avoids changing decoders and fixes the race condition IMHO since now AVCodecContext does not get updated by the demuxer but the application. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 12:09:36PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 09:01:38PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 11:49:54AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:35 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:51:22AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 10:33 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote: > Hi, > > On 4/11/2009 3:19 PM, Reimar D?ffinger wrote: >> Hello, >> attached patch implements this. >> It is a bit ugly because it adds a lot of seeking to the demuxer, but I >> think for such a fringe format there is no point in extra work for avoiding it. >> The patch also adds two helper functions, one to increase the size of a >> packet and one that appends data read from a file to a possibly already >> filled packet. >> I expect that there will be more that a few bugs still, e.g. while the >> video looks visually ok in ffplay the FATE regressions do not match at >> all. > Well it seems that palette could be stored in extradata in it's read in > wv3_read_header, a way to pass palette change information through > AVPacket would be needed. > > Also AVPacket could have ->palette_data field which seems a lot cleaner > to me and Ronald. palette_data would be a nightmare in pretty much every aspect first many formats allow multiple palette chunks to update a palette for a frame. Yes AVI too which i had forgotten, it too can update 2 entries in a palette through 2 pc chunks that each changes just 1 entry. Please enumerate the formats. State exactly what the problem is.
i think i did
Doesn't avi use bmp header ? If so palette -> extradata.
if you dont belive me you can read the source, look at sample files (like CIMOVI01.avi) or the avi spec
second, muxers that do not support such palettes (mov/mp4/asf?/rm/mkv/nut/...) must somehow merge them and this should really have been done in the demuxer already. I'm not sure about this, in _any_ case -vcodec copy will cause problems since you have merged the palette and the data in the packet.
-vcodec copy will not cause problems if muxers and demuxers implement the API correctly, this is true for any not to crappy API.
Parsing packet data is annoying in any case and IMHO it will only
well the palette chunks need to be parsed as they arent complete palettes you cant avoid that
Yes you can. If you don't append the palette to packet data you don't need to parse the packet.
you dont even need to decode it or display it because without the palette it wouldnt look too good anyway [...]
If the muxer can handle palette seperatly (mov does) it can create multiple stsd with correct palette information. It avoids parsing palette in the packet which is _really_ ugly.
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
Well it will work for sure.
how exactly would the libvaformat mov demuxer pass these palettes to the decoder?
Using the same way it does currently ?
You mean the one that causes race conditions and doesnt work with threaded players like ffplay ... The one why we try to change because it doesnt work ... [...]
[...]
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much
I don't forget, application will update AVCodecContext palette accordingly, of course. This does not remove palette support at all, and avoids changing decoders and fixes the race condition IMHO since now AVCodecContext does not get updated by the demuxer but the application.
Which application? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
On Wed, Apr 15, 2009 at 11:55:48PM +0200, Michael Niedermayer wrote:
[....]
the first sample file i picked has 87 palette chunks and a duration of 7.8 seconds (at 12fps) are you sure that 3600*30 stsd chunks in mov will work?
Well it will work for sure.
how exactly would the libvaformat mov demuxer pass these palettes to the decoder?
Using the same way it does currently ?
You mean the one that causes race conditions and doesnt work with threaded players like ffplay ... The one why we try to change because it doesnt work ...
ffmpeg kinda works. Yes there is a race problem and there are some ways to fix it. I strongly believe we can reach an agreement here.
[...]
[...]
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much
I don't forget, application will update AVCodecContext palette accordingly, of course. This does not remove palette support at all, and avoids changing decoders and fixes the race condition IMHO since now AVCodecContext does not get updated by the demuxer but the application.
Which application?
One you don't know about ? ;) This is still one solution. There are at least 3 solutions: - append palette to pkt->data - use pkt->palette_data - use AVFormatContext palette If the third is not ok for anybody then fine. I find the second reasonable and should satisfy everybody IMHO. Palette is passed through AVPacket and it avoids parsing in pkt->data, though it grows a bit AVPacket struct. It's not like we are afraid of growing structs usually ;) Besides, avcodec_decode_video2 now uses AVPacket. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 03:39:50PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:55:48PM +0200, Michael Niedermayer wrote: [...]
[...]
[...]
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much
I don't forget, application will update AVCodecContext palette accordingly, of course. This does not remove palette support at all, and avoids changing decoders and fixes the race condition IMHO since now AVCodecContext does not get updated by the demuxer but the application.
Which application?
One you don't know about ? ;)
Yes, exactly it will not work with any application i do know about ffplay, ffmpeg, vlc, mplayer, xine, ...
This is still one solution.
There are at least 3 solutions: - append palette to pkt->data
yes, thats what i suggested
- use pkt->palette_data
this is the same as using pkt->data, it needs grow and append and parsing read what reimar wrote and it does not work with most muxers as is
- use AVFormatContext palette
This is equivalent to what we do currently as i said already
If the third is not ok for anybody then fine.
I find the second reasonable and should satisfy everybody IMHO. Palette is passed through AVPacket and it avoids parsing in pkt->data, though it grows a bit AVPacket struct. It's not like we are afraid of growing structs usually ;)
you really arent reading what others write let me repeat formats store several compressed palette chunks that update the previous these need merging at the demuxer and parsing at the decoder no matter if they are in data or palette_data also the palette stuff no matter where it is need to be decompressed before you have 256 entries you can pass to the video out Decompression belongs to the decoder. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is not what we do, but why we do it that matters.
On Thu, Apr 16, 2009 at 01:08:41AM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:39:50PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:55:48PM +0200, Michael Niedermayer wrote: [...]
[...]
[...]
IIRC the race condition happens because demuxer sets palette to AVCodecContext _directly_.
Storing it in AVFormatContext should avoid this race, or is there something I'm forgetting ?
you forget that the palette from AVFormatContext must reach the decoder and what you do is that you would just remove the code that does that that fixes the race condition but it removes palette support pretty much
I don't forget, application will update AVCodecContext palette accordingly, of course. This does not remove palette support at all, and avoids changing decoders and fixes the race condition IMHO since now AVCodecContext does not get updated by the demuxer but the application.
Which application?
One you don't know about ? ;)
Yes, exactly it will not work with any application i do know about ffplay, ffmpeg, vlc, mplayer, xine, ...
Well since fixing AVPalette took several years. I would have thought player applications to implement the correct solution that is copying palette from AVCodecContext when palette_change is set.
This is still one solution.
There are at least 3 solutions: - append palette to pkt->data
yes, thats what i suggested
- use pkt->palette_data
this is the same as using pkt->data, it needs grow and append and parsing read what reimar wrote and it does not work with most muxers as is
Well I don't see how, sorry.
- use AVFormatContext palette
This is equivalent to what we do currently as i said already
Well messing with AVCodecContext when avformat is supposed to handle this can seen as awkward I agree.
If the third is not ok for anybody then fine.
I find the second reasonable and should satisfy everybody IMHO. Palette is passed through AVPacket and it avoids parsing in pkt->data, though it grows a bit AVPacket struct. It's not like we are afraid of growing structs usually ;)
you really arent reading what others write
let me repeat formats store several compressed palette chunks that update the previous these need merging at the demuxer and parsing at the decoder no matter if they are in data or palette_data
Yes, but storing this merged palette in pkt->palette_data instead of pkt->data appears simpler to Ronald and me. I'll repeat what I just said: if pkt->palette_data then extract_palette in decoder if pkt->data then damn is there a palette in here ? How do I know ? [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 04:35:04PM -0700, Baptiste Coudurier wrote:
On Thu, Apr 16, 2009 at 01:08:41AM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:39:50PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:55:48PM +0200, Michael Niedermayer wrote: [...]
If the third is not ok for anybody then fine.
I find the second reasonable and should satisfy everybody IMHO. Palette is passed through AVPacket and it avoids parsing in pkt->data, though it grows a bit AVPacket struct. It's not like we are afraid of growing structs usually ;)
you really arent reading what others write
let me repeat formats store several compressed palette chunks that update the previous these need merging at the demuxer and parsing at the decoder no matter if they are in data or palette_data
Yes, but storing this merged palette in pkt->palette_data instead of pkt->data appears simpler to Ronald and me.
I'll repeat what I just said:
if pkt->palette_data then extract_palette in decoder
that first requires demuxers to extract the palette of the stored packets and muxers to merge palette&data back Theres no advantage of this, and i must admit i nearly lost any interrest in fixing palettes, this discussion is so idiotic The whole issue exists since years, and when finally 2 people start working on fixing it you block all work with this discussion about where to store the palette. We have an existing API and that puts all of the packet in pkt->data, generic muxers & demuxers expect this, your suggestion would require a change of this API and IMO not to the better.
if pkt->data then damn is there a palette in here ? How do I know ?
same as how mpeg knows that theres a quant table or how it knows if its a b frame or p frame. Its written in pkt->data [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote:
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all.
Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
I don't see a point in inventing a new encoding format when it's all been done already, I am just moving a lot of parsing and decoding functions from the demuxer to the decoder where they would have belonged in the first place. The extradata idea _might_ work for wc3, but it would leave exactly the "same" issues with other formats like idcin plus due to lack of specification we don't know if new palettes can't be added in the middle of the file.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.).
On 4/15/2009 10:43 AM, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 10:22:26AM -0700, Baptiste Coudurier wrote:
On 4/11/2009 3:19 PM, Reimar D?ffinger wrote:
attached patch implements this. It is a bit ugly because it adds a lot of seeking to the demuxer, but I think for such a fringe format there is no point in extra work for avoiding it. The patch also adds two helper functions, one to increase the size of a packet and one that appends data read from a file to a possibly already filled packet. I expect that there will be more that a few bugs still, e.g. while the video looks visually ok in ffplay the FATE regressions do not match at all. Well it seems that palette could be stored in extradata in it's read in wv3_read_header, a way to pass palette change information through AVPacket would be needed.
I don't see a point in inventing a new encoding format when it's all been done already, I am just moving a lot of parsing and decoding functions from the demuxer to the decoder where they would have belonged in the first place. The extradata idea _might_ work for wc3, but it would leave exactly the "same" issues with other formats like idcin plus due to lack of specification we don't know if new palettes can't be added in the middle of the file.
A survey must be conducted. There is no point going to an ugly solution until we have a good picture of what is really needed. I'm pretty this av_grow_packet ugliness is useless.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.).
Please enumerate the formats and problems it might cause. Muxing to .mov is pretty much different than .mkv and stream copy case is important too, by altering the packet you are causing many sides effects. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
I'm pretty this av_grow_packet ugliness is useless.
It is needed because the formats were never designed for split demuxer and decoder. Without this you get stuff like in ipmovie:
av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size) url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data, s->decode_map_chunk_size); url_fseek(pb, s->video_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
Or in most cases simply an incomplete and sometimes subtly broken but at least inconsistent ad-hoc reimplementation of it. Suggestions of better solutions are of course welcome, I already asked for them. Twice.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald.
Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.).
Please enumerate the formats and problems it might cause. Muxing to .mov is pretty much different than .mkv and stream copy case
Well, maybe I am clueless, but I see nothing to enumerate, the issue is simply "where the heck do you store the data in palette_data when muxing?".
is important too, by altering the packet you are causing many sides effects.
Please be more specific. It is the _current_ demuxer code that completely munges up the data, throwing away information how the palette was encoded, sometimes even applying (lossy!) gamma correction and in the process, my patch simply changes that so that the demuxer passes all video-related data _unchanged_ to the decoder. The only issue is that the format might looks e.g. like this: fixed size video-related data palette size palette size variable size video data I guess the demuxer could just pick out those sizes via seeking, then seeking back and read them all in a bunch in a video packet but 1) it's not what I'd consider a better/cleaner solution 2) not sure if there aren't any not-video-related parts in-between ever...
On 4/15/2009 11:18 AM, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
I'm pretty this av_grow_packet ugliness is useless.
It is needed because the formats were never designed for split demuxer and decoder. Without this you get stuff like in ipmovie:
av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size) url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data, s->decode_map_chunk_size); url_fseek(pb, s->video_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
Or in most cases simply an incomplete and sometimes subtly broken but at least inconsistent ad-hoc reimplementation of it. Suggestions of better solutions are of course welcome, I already asked for them. Twice.
Well I already gave one: use extradata if possible.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald. Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.). Please enumerate the formats and problems it might cause. Muxing to .mov is pretty much different than .mkv and stream copy case
Well, maybe I am clueless, but I see nothing to enumerate, the issue is simply "where the heck do you store the data in palette_data when muxing?".
Well, IMHO you need to enumarate which codecs use palette and how do they store this information. After that you check how palette is supposed to be stored in the destination container, because containers may have a standard way to store palette. You can then decide how it seems usual and simple to store the palette.
is important too, by altering the packet you are causing many sides effects.
Please be more specific. It is the _current_ demuxer code that completely munges up the data, throwing away information how the palette was encoded, sometimes even applying (lossy!) gamma correction and in the process, my patch simply changes that so that the demuxer passes all video-related data _unchanged_ to the decoder. The only issue is that the format might looks e.g. like this: fixed size video-related data palette size palette size variable size video data
AVPacket->palette_data is still simpler IMHO. Also now that we pass AVPacket to decode_video, we may put AVPalette in AVFormatContext and use the same api, and instruct that palette changed to the decoder with a flag in AVPacket.
I guess the demuxer could just pick out those sizes via seeking, then seeking back and read them all in a bunch in a video packet but 1) it's not what I'd consider a better/cleaner solution 2) not sure if there aren't any not-video-related parts in-between ever...
Well, I'd say it depends on the way palette is stored. Also, IMHO a clear distinction must be done between libavformat and libavcodec, nothing should mandate the use of libavcodec decoder, although for these game formats libavcodec may be the only software decoder available. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 11:42:55AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:18 AM, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
I'm pretty this av_grow_packet ugliness is useless.
It is needed because the formats were never designed for split demuxer and decoder. Without this you get stuff like in ipmovie:
av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size) url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data, s->decode_map_chunk_size); url_fseek(pb, s->video_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
Or in most cases simply an incomplete and sometimes subtly broken but at least inconsistent ad-hoc reimplementation of it. Suggestions of better solutions are of course welcome, I already asked for them. Twice.
Well I already gave one: use extradata if possible.
There is no palette at all involved here.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald. Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.). Please enumerate the formats and problems it might cause. Muxing to .mov is pretty much different than .mkv and stream copy case
Well, maybe I am clueless, but I see nothing to enumerate, the issue is simply "where the heck do you store the data in palette_data when muxing?".
Well, IMHO you need to enumarate which codecs use palette and how do they store this information. After that you check how palette is supposed to be stored in the destination container, because containers may have a standard way to store palette.
You can then decide how it seems usual and simple to store the palette.
I don't really consider it worth the effort. Do you even know a container that supports 6-bit VGA palette? Or palette that needs gamma correction? A list of 16 or so palettes might be possible... And you haven't suggested what to when the container does not offer any suitable way to store that palette.
is important too, by altering the packet you are causing many sides effects.
Please be more specific. It is the _current_ demuxer code that completely munges up the data, throwing away information how the palette was encoded, sometimes even applying (lossy!) gamma correction and in the process, my patch simply changes that so that the demuxer passes all video-related data _unchanged_ to the decoder. The only issue is that the format might looks e.g. like this: fixed size video-related data palette size palette size variable size video data
AVPacket->palette_data is still simpler IMHO.
Huh? How would it be simpler? Instead of just passing the whole bunch of data on, the demuxer now has to extract the palette. Ok, otherwise the decoder has to extract, still seems pretty much the same to me. That is of course unless you absolutely insist on not providing any API packets to merge data together into a single AVPacket.
Also now that we pass AVPacket to decode_video, we may put AVPalette in AVFormatContext and use the same api, and instruct that palette changed to the decoder with a flag in AVPacket.
Oh dear. Are you even aware of the issues with AVPaletteControl? Because your suggestion works just as badly with buffering an multithreading that we could just keep it.
Also, IMHO a clear distinction must be done between libavformat and libavcodec, nothing should mandate the use of libavcodec decoder, although for these game formats libavcodec may be the only software decoder available.
I can't see how that speaks in favour of your suggestion, my changes make the decoder pass stuff on with as little modification as possible whereas your suggestion to me just like the current code seems likely to do palette decoding in the demuxer, thus linking demuxer and decoder much more together. Though I can't tell since you have not answered what palette_data is supposed to contain exactly.
On Wed, Apr 15, 2009 at 09:00:36PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 11:42:55AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:18 AM, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
I'm pretty this av_grow_packet ugliness is useless.
It is needed because the formats were never designed for split demuxer and decoder. Without this you get stuff like in ipmovie:
av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size) url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data, s->decode_map_chunk_size); url_fseek(pb, s->video_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
Or in most cases simply an incomplete and sometimes subtly broken but at least inconsistent ad-hoc reimplementation of it. Suggestions of better solutions are of course welcome, I already asked for them. Twice.
Well I already gave one: use extradata if possible.
There is no palette at all involved here.
In WC3 ? I thought you were talking about this. For WC3 I believe extradata can be used, maybe not but only a try can tell us.
Also AVPacket could have ->palette_data field which seems a lot cleaner to me and Ronald. Sure it is because you are completely ignoring the cases where it doesn't work (muxing to non-AVI always, and either you have to decode the palette etc. in the demuxer or it won't even work for remuxing into AVI - muxing for almost all these formats in the only way to get an index and thus quick and reliable seeking btw.). Please enumerate the formats and problems it might cause. Muxing to .mov is pretty much different than .mkv and stream copy case
Well, maybe I am clueless, but I see nothing to enumerate, the issue is simply "where the heck do you store the data in palette_data when muxing?".
Well, IMHO you need to enumarate which codecs use palette and how do they store this information. After that you check how palette is supposed to be stored in the destination container, because containers may have a standard way to store palette.
You can then decide how it seems usual and simple to store the palette.
I don't really consider it worth the effort.
I really consider it worth the effort.
Do you even know a container that supports 6-bit VGA palette? Or palette that needs gamma correction? A list of 16 or so palettes might be possible...
I don't know but the survey might help us knowing this if some people do know.
And you haven't suggested what to when the container does not offer any suitable way to store that palette.
Well, some containers aren't suitable, that's all, it's like vorbis in ts. No big deal IMHO.
is important too, by altering the packet you are causing many sides effects.
Please be more specific. It is the _current_ demuxer code that completely munges up the data, throwing away information how the palette was encoded, sometimes even applying (lossy!) gamma correction and in the process, my patch simply changes that so that the demuxer passes all video-related data _unchanged_ to the decoder. The only issue is that the format might looks e.g. like this: fixed size video-related data palette size palette size variable size video data
AVPacket->palette_data is still simpler IMHO.
Huh? How would it be simpler? Instead of just passing the whole bunch of data on, the demuxer now has to extract the palette. Ok, otherwise the decoder has to extract, still seems pretty much the same to me. That is of course unless you absolutely insist on not providing any API packets to merge data together into a single AVPacket.
Also now that we pass AVPacket to decode_video, we may put AVPalette in AVFormatContext and use the same api, and instruct that palette changed to the decoder with a flag in AVPacket.
Oh dear. Are you even aware of the issues with AVPaletteControl? Because your suggestion works just as badly with buffering an multithreading that we could just keep it.
Maybe not, what are the issues ? Please explain. I understand the problem with race condition is because the demuxer actually updates AVCodecContext palette directly, but IMHO that can be fixed by letting application dealing with palette change. If formats didn't judge pertinent to store the palette within the frame, why are we obsessed in doing this ?
Also, IMHO a clear distinction must be done between libavformat and libavcodec, nothing should mandate the use of libavcodec decoder, although for these game formats libavcodec may be the only software decoder available.
I can't see how that speaks in favour of your suggestion, my changes make the decoder pass stuff on with as little modification as possible whereas your suggestion to me just like the current code seems likely to do palette decoding in the demuxer, thus linking demuxer and decoder much more together. Though I can't tell since you have not answered what palette_data is supposed to contain exactly.
Well, your patch assumes that libavcodec is used unfortunately, and that libs are updated at the same time too. If we don't decode palette in demuxer, we could pass raw data to decoder in some way. I just don't like appending palette in packet data because this forces parsing. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 12:21:57PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 09:00:36PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 11:42:55AM -0700, Baptiste Coudurier wrote:
On 4/15/2009 11:18 AM, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
I'm pretty this av_grow_packet ugliness is useless.
It is needed because the formats were never designed for split demuxer and decoder. Without this you get stuff like in ipmovie:
av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size) url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data, s->decode_map_chunk_size); url_fseek(pb, s->video_chunk_offset, SEEK_SET); get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
Or in most cases simply an incomplete and sometimes subtly broken but at least inconsistent ad-hoc reimplementation of it. Suggestions of better solutions are of course welcome, I already asked for them. Twice.
Well I already gave one: use extradata if possible.
There is no palette at all involved here.
In WC3 ? I thought you were talking about this.
I was talking about the code I was quoting, as an example that I never intended this to be the only use for those API functions.
I don't really consider it worth the effort.
I really consider it worth the effort.
Then please go ahead.
I understand the problem with race condition is because the demuxer actually updates AVCodecContext palette directly, but IMHO that can be fixed by letting application dealing with palette change.
So your solution is just to remove the only way to pass the palette that is there? Sorry, I really don't see the point of continuing discussion if you only provide solutions that are so much simpler because they don't even half-work and in some other cases because you are free to choose whichever way is more convenient for them problem but exclude each other (e.g. "compressed" vs. uncompressed palette data). It's just another bikeshed like that and I'm really tired of the endless discussions it causes with in the end nobody doing any fix whatsoever, just like in the DV demuxer case.
If formats didn't judge pertinent to store the palette within the frame, why are we obsessed in doing this ?
_I_ am not talking about AVI, for the other formats they can be just as much considered "within" the frame as they could be considered separate IMO.
Well, your patch assumes that libavcodec is used unfortunately,
No.
and that libs are updated at the same time too.
As would any change removing AVPaletteControl
If we don't decode palette in demuxer, we could pass raw data to decoder in some way.
For example in the way my patch does
I just don't like appending palette in packet data because this forces parsing.
Again, _I_ am not considering AVI/ASF and I make no claim as to what the proper solution for them is, but _my_ patch mostly only _moves_ code from the demuxer to the decoder.
On Wed, Apr 15, 2009 at 09:52:27PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 12:21:57PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 09:00:36PM +0200, Reimar D?ffinger wrote:
[...]
I don't really consider it worth the effort.
I really consider it worth the effort.
Then please go ahead.
Sure, I will.
I understand the problem with race condition is because the demuxer actually updates AVCodecContext palette directly, but IMHO that can be fixed by letting application dealing with palette change.
So your solution is just to remove the only way to pass the palette that is there?
Absolutely not.
Sorry, I really don't see the point of continuing discussion if you only provide solutions that are so much simpler because they don't even half-work and in some other cases because you are free to choose whichever way is more convenient for them problem but exclude each other (e.g. "compressed" vs. uncompressed palette data).
I don't agree with you. I propose alternatives but you just don't want to consider them. It's not what I call try to find compromises nor consensus. There are some way to avoid av_grow_packet. I proposed alternatives. Now please let's try to find a compromise. IMHO it's not fair nor justified to ignore Ronald's and my comments.
It's just another bikeshed like that and I'm really tired of the endless discussions it causes with in the end nobody doing any fix whatsoever, just like in the DV demuxer case.
Well IMHO adding another demuxer is poluting allcodecs/makefiles and is not needed. But don't worry I will eventually fix it myself ;)
If formats didn't judge pertinent to store the palette within the frame, why are we obsessed in doing this ?
_I_ am not talking about AVI, for the other formats they can be just as much considered "within" the frame as they could be considered separate IMO.
Well, your patch assumes that libavcodec is used unfortunately,
No.
Yes it does.
and that libs are updated at the same time too.
As would any change removing AVPaletteControl
Well I proposed an alternative which would not remove AVPaletteControl.
If we don't decode palette in demuxer, we could pass raw data to decoder in some way.
For example in the way my patch does
I just don't like appending palette in packet data because this forces parsing.
Again, _I_ am not considering AVI/ASF and I make no claim as to what the proper solution for them is, but _my_ patch mostly only _moves_ code from the demuxer to the decoder.
Well code still appends (compressed) palette to packet data. As a side note, I don't think av_grow_packet, av_append_packet belongs to public API, nor av_shrink_packet but it's too late. Also WC3 case might use extradata to store palette this would solve the mov/avi/mkv case at first. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote: [very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header i have no real oppinion on doing this or not doing this but, this has absolutely nothing to do with palettes ... ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header
i have no real oppinion on doing this or not doing this
but, this has absolutely nothing to do with palettes ...
ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical.
How is this contructive at all ? What are you trying to achieve here ? Deliberately ignoring people ? -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header
i have no real oppinion on doing this or not doing this
but, this has absolutely nothing to do with palettes ...
ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical.
How is this contructive at all ? What are you trying to achieve here ? Deliberately ignoring people ?
Its constructive in the sense that implementing it will lead to working palette support. This discussion did not lead to any new insights in how palette support could be implemented in a way that works. I have considered the extra field and extra flag in AVPacket years ago, the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ... I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
On Wed, Apr 15, 2009 at 11:43:12PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header
i have no real oppinion on doing this or not doing this
but, this has absolutely nothing to do with palettes ...
ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical.
How is this contructive at all ? What are you trying to achieve here ? Deliberately ignoring people ?
Its constructive in the sense that implementing it will lead to working palette support. This discussion did not lead to any new insights in how palette support could be implemented in a way that works.
I have considered the extra field and extra flag in AVPacket years ago,
What are the disadvantages of pkt->palette_data ? It's one more field which Ronald and me would prefer vs merging palette with pkt->data which requires parsing. I don't think that deliberately forcing your opinion is constructive in any way.
the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ...
No, we do not use a separate context for format _and_ codec currently. That's one other option.
[...]
I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion.
Well, I think you are. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 03:21:53PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:43:12PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header
i have no real oppinion on doing this or not doing this
but, this has absolutely nothing to do with palettes ...
ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical.
How is this contructive at all ? What are you trying to achieve here ? Deliberately ignoring people ?
Its constructive in the sense that implementing it will lead to working palette support. This discussion did not lead to any new insights in how palette support could be implemented in a way that works.
I have considered the extra field and extra flag in AVPacket years ago,
What are the disadvantages of pkt->palette_data ? It's one more field which Ronald and me would prefer vs merging palette with pkt->data which requires parsing.
i alraedy said it needs parsing too
I don't think that deliberately forcing your opinion is constructive in any way.
And i think deliberately blocking a solution without offering any consistent alternative is very trollish behavior.
the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ...
No, we do not use a separate context for format _and_ codec currently. That's one other option.
Its of no relevance at all if the context is seperate, an application could take the palette out of AVCodecContext as much as it could out of AVFormatContext, theres no difference at all both are available to an application. But applications do NOT do it because its a huge mess to implement this in a way that works, instead applications take the palette out of AVFrame.data which is not thread safe and does not work.
[...]
I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion.
Well, I think you are.
You had trolled around like this in the past mjpeg, gif palette, ... and in no case did the discussions lead to anything also reimar and me asked you several times to describe your alternaitve you did not. Theres nothing i ignore you just did not describe anything that is self consistent and non ambigous. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
On Thu, Apr 16, 2009 at 12:53:53AM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:21:53PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:43:12PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[very boring "discussion"]
public API, nor av_shrink_packet but it's too late.
a patch moving av_shrink_packet() to ff_shrink_packet() in lavf is welcome av_shrink_packet() could be put under the appropriate #if version and removed from the public header
i have no real oppinion on doing this or not doing this
but, this has absolutely nothing to do with palettes ...
ontopic: ill fix the avi demuxer as soon as the append packet code is in svn. A solution with extradata is not possible for avi, nor is one based on passing the palette through AVFormatContext practical.
How is this contructive at all ? What are you trying to achieve here ? Deliberately ignoring people ?
Its constructive in the sense that implementing it will lead to working palette support. This discussion did not lead to any new insights in how palette support could be implemented in a way that works.
I have considered the extra field and extra flag in AVPacket years ago,
What are the disadvantages of pkt->palette_data ? It's one more field which Ronald and me would prefer vs merging palette with pkt->data which requires parsing.
i alraedy said it needs parsing too
If pkt->palette_data then extract_palette ? if pkt->data ? argh damn does this packet contains palette or not ? I just don't know.
I don't think that deliberately forcing your opinion is constructive in any way.
And i think deliberately blocking a solution without offering any consistent alternative is very trollish behavior.
Sorry but I don't think it is the case. I just enumerated 3 solutions including yours.
the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ...
No, we do not use a separate context for format _and_ codec currently. That's one other option.
Its of no relevance at all if the context is seperate, an application could take the palette out of AVCodecContext as much as it could out of AVFormatContext, theres no difference at all both are available to an application. But applications do NOT do it because its a huge mess to implement this in a way that works, instead applications take the palette out of AVFrame.data which is not thread safe and does not work.
Well internalizing because it is a big mess to implement can IMHO be a good think of course. Now if you buffer pkt you buffer pkt->palette_data and packet is passed to the decoder using avcodec_decode_video2. What's the problem here ?
[...]
I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion.
Well, I think you are.
You had trolled around like this in the past mjpeg, gif palette, ...
You call discussing, trolling. The discussion was justified IMHO. Besides this was not about gif palette, it was how to implement animated gif in the decoder. This is not the first time you call trolling people expressing their opinion which is against yours. About gif animated, you just did remove the feature.
and in no case did the discussions lead to anything
Of course it did, mjpeg interlacing is somehow fixed, not correctly since it forces a multiple of 2 height, but it somehow works. If this discussion didn't happen bug report would be still rotting in roundup.
also reimar and me asked you several times to describe your alternaitve you did not.
Sorry, but I think I just did in the last mail. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 04:24:34PM -0700, Baptiste Coudurier wrote:
On Thu, Apr 16, 2009 at 12:53:53AM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:21:53PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:43:12PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[...]
the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ...
No, we do not use a separate context for format _and_ codec currently. That's one other option.
Its of no relevance at all if the context is seperate, an application could take the palette out of AVCodecContext as much as it could out of AVFormatContext, theres no difference at all both are available to an application. But applications do NOT do it because its a huge mess to implement this in a way that works, instead applications take the palette out of AVFrame.data which is not thread safe and does not work.
Well internalizing because it is a big mess to implement can IMHO be a good think of course.
Now if you buffer pkt you buffer pkt->palette_data and packet is passed to the decoder using avcodec_decode_video2.
What's the problem here ?
That user apps that are not based on libav* (mplayer, xine, vlc at least) dont use AVPacket internally, and i suspect many players will rather drop palette support or even libavformat support before redesigning their internal buffer and fifo handling to pass vectors of arrays between demuxers and decoders. Also there are systems (maybe dshow?) that are not going to be redesiged to allow passing of vectors of arrays between layers we are not in power to choose any arbitrary interface between demuxer and decoder because players likely have their own interface and just use libavcodec and libavformat in addition to their own native system. Besides we should not change the interface without exceedingly good reason
[...]
I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion.
Well, I think you are.
You had trolled around like this in the past mjpeg, gif palette, ...
You call discussing, trolling. The discussion was justified IMHO. Besides this was not about gif palette, it was how to implement animated gif in the decoder.
there where multiple gif trollings i guess ;) one was about how the bugs in the gif decoder where twisted by you into bugs in swscale and effectively forced me to fix your code to proof you wrong. [...]
and in no case did the discussions lead to anything
Of course it did, mjpeg interlacing is somehow fixed, not correctly since it forces a multiple of 2 height, but it somehow works.
its fixed because i fixed it [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire
Hi Michael, On 4/16/2009 6:48 AM, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 04:24:34PM -0700, Baptiste Coudurier wrote:
On Thu, Apr 16, 2009 at 12:53:53AM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 03:21:53PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:43:12PM +0200, Michael Niedermayer wrote:
On Wed, Apr 15, 2009 at 01:51:32PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 10:32:29PM +0200, Michael Niedermayer wrote: > On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
[...]
the global context doesnt work as the palette isnt global, and using the context is what we do currently and that causes race conditions, which is why its currently not working ... No, we do not use a separate context for format _and_ codec currently. That's one other option. Its of no relevance at all if the context is seperate, an application could take the palette out of AVCodecContext as much as it could out of AVFormatContext, theres no difference at all both are available to an application. But applications do NOT do it because its a huge mess to implement this in a way that works, instead applications take the palette out of AVFrame.data which is not thread safe and does not work. Well internalizing because it is a big mess to implement can IMHO be a good think of course.
Now if you buffer pkt you buffer pkt->palette_data and packet is passed to the decoder using avcodec_decode_video2.
What's the problem here ?
That user apps that are not based on libav* (mplayer, xine, vlc at least) dont use AVPacket internally, and i suspect many players will rather drop palette support or even libavformat support before redesigning their internal buffer and fifo handling to pass vectors of arrays between demuxers and decoders.
Indeed that's a good point. Do you think they will craft special data needed by decoders like palette ? For gif in .mov, if quicktime is used with perian, you will get in the decoder what quicktime demux. This means palette will be in extradata/stsd structure. For palette in .avi and .wmv, if windows media player is used, you will get what wmp give to the windows decoder api. This means palette will be in bmp header. Game formats case may be different since not many players support them. Now when the palette is within the bitstream itself interleaved with video packets, it makes sense to demux these packets "as is" to the decoder. Do they need to be merged with the video packet ? Maybe, maybe not, decode_video can return 0 but not set got_picture, decoders should be already handling this. When it's not interleaved but pointers to specific palettes in header are in the bitstream (like WC3), it may be worth using extradata. This may make seeking and streaming support easier, but there is still a need to inform the decoder that palette has changed. AVCodecContext field could be used in this case. This is a quick picture, and of course is not exhaustive. Any other information is welcome.
Also there are systems (maybe dshow?) that are not going to be redesiged to allow passing of vectors of arrays between layers
we are not in power to choose any arbitrary interface between demuxer and decoder because players likely have their own interface and just use libavcodec and libavformat in addition to their own native system. Besides we should not change the interface without exceedingly good reason
This is probably true, see above.
[...]
I do NOT ignore suggestions that have advantages but I do ignore equivalent suggestions that have no advantages, i rather move forward if noone has a better suggestion.
Well, I think you are. You had trolled around like this in the past mjpeg, gif palette, ... You call discussing, trolling. The discussion was justified IMHO. Besides this was not about gif palette, it was how to implement animated gif in the decoder.
there where multiple gif trollings i guess ;) one was about how the bugs in the gif decoder where twisted by you into bugs in swscale and effectively forced me to fix your code to proof you wrong.
Sorry for having been so dumb to fail to understand PIX_FMT_PAL8 usage.
and in no case did the discussions lead to anything Of course it did, mjpeg interlacing is somehow fixed, not correctly since it forces a multiple of 2 height, but it somehow works.
its fixed because i fixed it
I know you are the best. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
I understand the problem with race condition is because the demuxer actually updates AVCodecContext palette directly, but IMHO that can be fixed by letting application dealing with palette change.
So your solution is just to remove the only way to pass the palette that is there?
Absolutely not.
Then how?
Sorry, I really don't see the point of continuing discussion if you only provide solutions that are so much simpler because they don't even half-work and in some other cases because you are free to choose whichever way is more convenient for them problem but exclude each other (e.g. "compressed" vs. uncompressed palette data).
I don't agree with you. I propose alternatives but you just don't want to consider them. It's not what I call try to find compromises nor consensus.
Because to me they look flawed front to end and are not even yet finished enough to be code.
There are some way to avoid av_grow_packet. I proposed alternatives. Now please let's try to find a compromise.
I asked I think now three or 4 times for alternatives to av_grow_packet that allows assembling a single packet from different locations in a file. I quoted one piece of code that could make good use of it and I doubt it will be the only one. I also suggest a way to do av_append_packet without av_grow_packet, but nobody responded either.
IMHO it's not fair nor justified to ignore Ronald's and my comments.
Given above I can't see you doing any better, and I have been writing actual code to solve a problem that has been ignored for months (years?), has already the maintainers ok and was ignored for days.
It's just another bikeshed like that and I'm really tired of the endless discussions it causes with in the end nobody doing any fix whatsoever, just like in the DV demuxer case.
Well IMHO adding another demuxer is poluting allcodecs/makefiles and is not needed.
I sent _two_ different solutions, It still did not get anywhere.
Well, your patch assumes that libavcodec is used unfortunately,
No.
Yes it does.
No it doesn't. Shall we go on like that some longer or are you going to start to explain your ideas in a way that makes it possible for others to follow?
and that libs are updated at the same time too.
As would any change removing AVPaletteControl
Well I proposed an alternative which would not remove AVPaletteControl.
I can't see how it should work without changing applications though. I admit I have not considered how mixing lav* versions would work with it.
As a side note, I don't think av_grow_packet, av_append_packet belongs to public API, nor av_shrink_packet but it's too late.
The decision was that libav* should only use public functions from other libav*, in that case there is no choice.
On Wed, Apr 15, 2009 at 11:12:04PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote: [...]
As a side note, I don't think av_grow_packet, av_append_packet belongs to public API, nor av_shrink_packet but it's too late.
The decision was that libav* should only use public functions from other libav*, in that case there is no choice.
we only need these things in libavformat thus they could be private in there [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On Wed, Apr 15, 2009 at 11:12:04PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
I understand the problem with race condition is because the demuxer actually updates AVCodecContext palette directly, but IMHO that can be fixed by letting application dealing with palette change.
So your solution is just to remove the only way to pass the palette that is there?
Absolutely not.
Then how?
Well stricly speaking it's not removing anything. My solution is extending using AVFormatContext to export the palette and letting application updating AVCodecContext accordingly. Or using pkt->palette_data and using avcodec_decode_video2. This solution is a compromise IMHO.
Sorry, I really don't see the point of continuing discussion if you only provide solutions that are so much simpler because they don't even half-work and in some other cases because you are free to choose whichever way is more convenient for them problem but exclude each other (e.g. "compressed" vs. uncompressed palette data).
I don't agree with you. I propose alternatives but you just don't want to consider them. It's not what I call try to find compromises nor consensus.
Because to me they look flawed front to end and are not even yet finished enough to be code.
I don't think so.
There are some way to avoid av_grow_packet. I proposed alternatives. Now please let's try to find a compromise.
I asked I think now three or 4 times for alternatives to av_grow_packet that allows assembling a single packet from different locations in a file. I quoted one piece of code that could make good use of it and I doubt it will be the only one. I also suggest a way to do av_append_packet without av_grow_packet, but nobody responded either.
I believe pkt->palette_data will avoid av_append and av_grow.
IMHO it's not fair nor justified to ignore Ronald's and my comments.
Given above I can't see you doing any better, and I have been writing actual code to solve a problem that has been ignored for months (years?), has already the maintainers ok and was ignored for days.
Well I'm waiting for a consensus to be reached here. After this, of course I can start coding.
It's just another bikeshed like that and I'm really tired of the endless discussions it causes with in the end nobody doing any fix whatsoever, just like in the DV demuxer case.
Well IMHO adding another demuxer is poluting allcodecs/makefiles and is not needed.
I sent _two_ different solutions, It still did not get anywhere.
Well sorry then, if you have one solution fixing the problem without using another demuxer, I think it should be fine. Is it really the case ?
Well, your patch assumes that libavcodec is used unfortunately,
No.
Yes it does.
No it doesn't. Shall we go on like that some longer or are you going to start to explain your ideas in a way that makes it possible for others to follow?
If it wasn't assuming this, why do you need to change the decoder at all ? How can you change the bitstream exported by the demuxer without breaking API ? Making it working without changing the decoder would be a proof that the demuxer is not assuming that libavcodec will be used. Someone could have workarounded the race condition by using separate AVCodecContext for demuxer and decoder for example, or using its own demuxer. Unlikely the case, but it might be possible.
and that libs are updated at the same time too.
As would any change removing AVPaletteControl
Well I proposed an alternative which would not remove AVPaletteControl.
I can't see how it should work without changing applications though. I admit I have not considered how mixing lav* versions would work with it.
Changing applications is needed because of avcodec_decode_video2 anyway.
The decision was that libav* should only use public functions from other libav*, in that case there is no choice.
av_get_packet is in libavformat only. grow and shrink could be also. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no FFmpeg maintainer http://www.ffmpeg.org
On Wed, Apr 15, 2009 at 03:33:29PM -0700, Baptiste Coudurier wrote:
On Wed, Apr 15, 2009 at 11:12:04PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 15, 2009 at 01:22:10PM -0700, Baptiste Coudurier wrote:
There are some way to avoid av_grow_packet. I proposed alternatives. Now please let's try to find a compromise.
I asked I think now three or 4 times for alternatives to av_grow_packet that allows assembling a single packet from different locations in a file. I quoted one piece of code that could make good use of it and I doubt it will be the only one. I also suggest a way to do av_append_packet without av_grow_packet, but nobody responded either.
I believe pkt->palette_data will avoid av_append and av_grow.
Sorry, but count me out. You obviously can't even be trying to read what I write, that is just a waste of time.
participants (3)
-
baptiste.coudurier＠gmail.com -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de