[PATCH] add alloc_put_byte function
Hello, since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this the actual ByteIOContext struct can be removed from the public API. Greetings, Reimar D?ffinger
On Wed, Dec 19, 2007 at 07:47:45PM +0100, Reimar D?ffinger wrote:
Hello, since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this
What about read_pause / read_seek ? Some things like MMS do not support seeking per positon AFAIK, and seeking per timestamp needs read_seek() Iam just curious, dont you consider that needed/usefull as well? Same for the ability to pause (tell the server not to send more packets) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
Hello, On Wed, Dec 19, 2007 at 10:48:50PM +0100, Michael Niedermayer wrote:
On Wed, Dec 19, 2007 at 07:47:45PM +0100, Reimar D?ffinger wrote:
since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this
What about read_pause / read_seek ?
Some things like MMS do not support seeking per positon AFAIK, and seeking per timestamp needs read_seek()
Iam just curious, dont you consider that needed/usefull as well? Same for the ability to pause (tell the server not to send more packets)
I mostly missed them, but I admit that I don't see MPlayer using them in the near future. I don't really like that alloc_put_byte function anyway, I just don't want to wait "forever" to find a good solution. Greetings, Reimar D?ffinger
Hello, On Wed, Dec 19, 2007 at 10:48:50PM +0100, Michael Niedermayer wrote:
On Wed, Dec 19, 2007 at 07:47:45PM +0100, Reimar D?ffinger wrote:
since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this
What about read_pause / read_seek ?
Some things like MMS do not support seeking per positon AFAIK, and seeking per timestamp needs read_seek()
Iam just curious, dont you consider that needed/usefull as well? Same for the ability to pause (tell the server not to send more packets)
After aurel's patch I think it might be better to have this alloc function accept the same things a init, and have a set_is_streamed and a set_streaming_functions or so functions to set the other things. Greetings, Reimar D?ffinger
On Wed, Dec 19, 2007 at 10:48:50PM +0100, Michael Niedermayer wrote:
On Wed, Dec 19, 2007 at 07:47:45PM +0100, Reimar D?ffinger wrote:
Hello, since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this
What about read_pause / read_seek ?
Some things like MMS do not support seeking per positon AFAIK, and seeking per timestamp needs read_seek()
Iam just curious, dont you consider that needed/usefull as well? Same for the ability to pause (tell the server not to send more packets)
Well i guess the question of which arguments the function should take is mostly bikeshed. So iam fine with the patch except that the function needs a av_ prefix [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
Hi Michael Niedermayer wrote:
On Wed, Dec 19, 2007 at 10:48:50PM +0100, Michael Niedermayer wrote:
On Wed, Dec 19, 2007 at 07:47:45PM +0100, Reimar D?ffinger wrote:
Hello, since applications should not assume ByteIOContext to have a constant size, attached patch adds a alloc_put_byte function. It is almost the same as init_put_byte, but it also takes a streamed_flag parameter - this seems the only missing option the user has a reason to set and it's exactly what I need, so like this
What about read_pause / read_seek ?
Some things like MMS do not support seeking per positon AFAIK, and seeking per timestamp needs read_seek()
Iam just curious, dont you consider that needed/usefull as well? Same for the ability to pause (tell the server not to send more packets)
Well i guess the question of which arguments the function should take is mostly bikeshed. So iam fine with the patch except that the function needs a av_ prefix
For consistency, can we choose a naming convention ? We got: avcodec_alloc_context av_alloc_format_context Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I'd prefer something like av_alloc_byteio_context. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hello, On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me.
I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-) Greetings, Reimar D?ffinger
Hello, On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me.
I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-)
Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ). Greetings, Reimar D?ffinger
Hi Reimar, Reimar D?ffinger a ?crit :
Hello, On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-)
Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ? -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA checking for life_signs in -lkenny... no
Baptiste Coudurier wrote:
Hi Reimar,
Reimar D?ffinger a ?crit :
Hello, On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-) Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ?
I'll apply the following patch in a few hours. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Baptiste Coudurier wrote:
Baptiste Coudurier wrote:
Hi Reimar,
Reimar D?ffinger a ?crit :
Hello, On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...] particularly either. I'll go with anything you agree on ;-) Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ?
I'll apply the following patch in a few hours.
[...]
Index: libavformat/avio.h =================================================================== --- libavformat/avio.h (revision 11525) +++ libavformat/avio.h (working copy) @@ -181,7 +181,7 @@ int (*read_packet)(void *opaque, uint8_t *buf, int buf_size), int (*write_packet)(void *opaque, uint8_t *buf, int buf_size), offset_t (*seek)(void *opaque, offset_t offset, int whence)); -ByteIOContext *av_alloc_put_byte( +ByteIOContext *av_alloc_byteio_context( unsigned char *buffer, int buffer_size, int write_flag,
I don't know if anyone care, but this breaks API (so this should require a major version bump). Aurel
Hi, Aurelien Jacobs wrote:
Baptiste Coudurier wrote:
Baptiste Coudurier wrote:
Hi Reimar,
Reimar D?ffinger a ?crit :
Hello, On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...] particularly either. I'll go with anything you agree on ;-) Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ?
I'll apply the following patch in a few hours.
[...]
Index: libavformat/avio.h =================================================================== --- libavformat/avio.h (revision 11525) +++ libavformat/avio.h (working copy) @@ -181,7 +181,7 @@ int (*read_packet)(void *opaque, uint8_t *buf, int buf_size), int (*write_packet)(void *opaque, uint8_t *buf, int buf_size), offset_t (*seek)(void *opaque, offset_t offset, int whence)); -ByteIOContext *av_alloc_put_byte( +ByteIOContext *av_alloc_byteio_context( unsigned char *buffer, int buffer_size, int write_flag,
I don't know if anyone care, but this breaks API (so this should require a major version bump).
Like Reimar pointed, since last change did not increase minor version (API extension), function cannot be considered as part of the API yet. This time it will be considered as API extension (because of the minor version bump). -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
On Mon, 14 Jan 2008, Baptiste Coudurier wrote:
Baptiste Coudurier wrote:
Reimar D?ffinger a ?crit :
On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-) Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ?
I'll apply the following patch in a few hours.
Hello, Any chance of this being applied? It's just I "accidentally" used the new function in some code I wrote for someone and so have had to send them a new version ready to be used when the function has been renamed - we're still waiting! Regards, Paul
Hi Paul Kelly wrote:
On Mon, 14 Jan 2008, Baptiste Coudurier wrote:
Baptiste Coudurier wrote:
Reimar D?ffinger a ?crit :
On Fri, Dec 21, 2007 at 09:49:05AM +0100, Reimar D?ffinger wrote:
On Thu, Dec 20, 2007 at 11:58:32PM +0100, Baptiste Coudurier wrote: [...]
For consistency, can we choose a naming convention ?
We got: avcodec_alloc_context av_alloc_format_context
Furthermore, ByteIOContext can be used to read data, so naming it alloc_put_byte seems somehow odd to me. I just went on analogy to init_put_byte, I don't like it particularly either. I'll go with anything you agree on ;-) Applied with only av_ prefix added, I do not want to wait forever with a depending patch. Feel free to come up with a better name and then bump the lavf version (I originally just forgot to do it, but now think it might be good as it give as chance to change the name without breaking API in the strict sense ;-) ).
Ok, I already suggested av_alloc_byteio_context, anyone against it ?
I'll apply the following patch in a few hours.
Hello, Any chance of this being applied? It's just I "accidentally" used the new function in some code I wrote for someone and so have had to send them a new version ready to be used when the function has been renamed - we're still waiting!
Well, since you are giving some kind of proof that it's actually being used outside, I therefore won't apply it without breaking API, so I think I won't apply it soon, or I'll apply something not breaking API. Nonetheless, lavf minor version must be bumped now. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hello, On Sun, Feb 03, 2008 at 05:32:35PM +0100, Baptiste Coudurier wrote:
Well, since you are giving some kind of proof that it's actually being used outside, I therefore won't apply it without breaking API, so I think I won't apply it soon, or I'll apply something not breaking API.
If you want to change it, just change it. Now! There is no point in dragging this on forever, and then change it when everyone and their dogs are using it instead of now, API compatibility is a tool for a goal, not something to keep out of principle.
Hi, Reimar Doeffinger wrote:
Hello, On Sun, Feb 03, 2008 at 05:32:35PM +0100, Baptiste Coudurier wrote:
Well, since you are giving some kind of proof that it's actually being used outside, I therefore won't apply it without breaking API, so I think I won't apply it soon, or I'll apply something not breaking API.
If you want to change it, just change it. Now! There is no point in dragging this on forever, and then change it when everyone and their dogs are using it instead of now, API compatibility is a tool for a goal, not something to keep out of principle.
This change is only to keep functions names consistent within libavformat. Now: 1) You applied it without considering my opinion just because you didn't want to delay another patch. this is clearly NOT acceptable. 2) You did extend API without bumping minor. 3) Aurel emitted doubts about API breaking, and I won't ignore other's opinions just because I want the change. 4) This once again proves that once patch hits svn it is really harder to change it. Now I consider, like Aurel, that this change will actually break API, so Im reconsidering the change, because it is only justified for the sake of naming consistency and will cause trouble for people already using it. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hello, On Sun, Feb 03, 2008 at 08:00:54PM +0100, Baptiste Coudurier wrote:
This change is only to keep functions names consistent within libavformat.
And how does letting inconsistent function names "ripe" help anyone? (though maybe the issue here is that it already is quite "ripe"). I do not consider one a lot more consistent than the other, but I happily go with what others say.
Now:
1) You applied it without considering my opinion just because you didn't want to delay another patch. this is clearly NOT acceptable.
I certainly did consider it. I explained why I chose the name, since I did not get a reply either way I went ahead with what I had. I admit that only just over 24 hours was not a long time to wait for a reply though, sorry. I just did not really want to wait to commit until mid-January and half-forgetting about the patch nor did I want to commit something right on the day before going on holidays.
2) You did extend API without bumping minor.
Which was intentional, I would have fixed that shortly later if there was not that pending naming issue. I did not expect it to stay like that for over a month (unfortunately I just forgot about this).
3) Aurel emitted doubts about API breaking, and I won't ignore other's opinions just because I want the change.
It seems he did not have all the details though. Though in the mean time the version number was changed, so the argument does not hold.
Now I consider, like Aurel, that this change will actually break API, so Im reconsidering the change, because it is only justified for the sake of naming consistency and will cause trouble for people already using it.
So how about #ifdef ing it for a major version number change? If you think the naming is inconsistent, just leaving it as it is, is no good either. Though I do not know if you consider it that inconsistent. Greetings, Reimar Doeffinger
Reimar Doeffinger wrote:
Hello, On Sun, Feb 03, 2008 at 08:00:54PM +0100, Baptiste Coudurier wrote:
3) Aurel emitted doubts about API breaking, and I won't ignore other's opinions just because I want the change.
It seems he did not have all the details though. Though in the mean time the version number was changed, so the argument does not hold.
Indeed.
Now I consider, like Aurel, that this change will actually break API, so Im reconsidering the change, because it is only justified for the sake of naming consistency and will cause trouble for people already using it.
I still consider it's breaking API. But I don't care at all about breaking API, as long as major version is increased at the same time.
So how about #ifdef ing it for a major version number change? If you think the naming is inconsistent, just leaving it as it is, is no good either. Though I do not know if you consider it that inconsistent.
I think the naming is inconsistent, and I agree it need to be changed. IMO, we should just increase major at the same time and be done with it. But if you prefer #ifdef ing it for next major version bump, I'm fine with it too. Aurel
Reimar Doeffinger wrote:
Hello, On Sun, Feb 03, 2008 at 08:00:54PM +0100, Baptiste Coudurier wrote:
This change is only to keep functions names consistent within libavformat.
And how does letting inconsistent function names "ripe" help anyone? (though maybe the issue here is that it already is quite "ripe"). I do not consider one a lot more consistent than the other, but I happily go with what others say.
Now:
1) You applied it without considering my opinion just because you didn't want to delay another patch. this is clearly NOT acceptable.
I certainly did consider it. I explained why I chose the name, since I did not get a reply either way I went ahead with what I had. I admit that only just over 24 hours was not a long time to wait for a reply though, sorry. I just did not really want to wait to commit until mid-January and half-forgetting about the patch nor did I want to commit something right on the day before going on holidays.
The problem I have here, is that I said on 12/20/2007 11:58 PM +0100: "I'd prefer something like av_alloc_byteio_context.", and clearly said that I found current name odd. You answered on 12/21/2007 09:49 AM +0100: "I'll go with anything you agree", and you nevertheless applied the old patch. Honestly I just think this is weird, because I had already proposed another name.
2) You did extend API without bumping minor.
Which was intentional, I would have fixed that shortly later if there was not that pending naming issue. I did not expect it to stay like that for over a month (unfortunately I just forgot about this).
Well, yes, due to vacation time it stood like that and honestly I did not want to fight about it either. So I think version should be bump in any case, it is even better to wait a bit before extending API, to let people settle and propose, discuss and agree.
3) Aurel emitted doubts about API breaking, and I won't ignore other's opinions just because I want the change.
It seems he did not have all the details though. Though in the mean time the version number was changed, so the argument does not hold.
Now I consider, like Aurel, that this change will actually break API, so Im reconsidering the change, because it is only justified for the sake of naming consistency and will cause trouble for people already using it.
So how about #ifdef ing it for a major version number change? If you think the naming is inconsistent, just leaving it as it is, is no good either. Though I do not know if you consider it that inconsistent.
Im fine with ifdef or deprecated attribute, but I don't want to break API again (and do not want to bump major and consequently soname). -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
participants (5)
-
aurel＠gnuage.org -
baptiste.coudurier＠smartjog.com -
michaelni＠gmx.at -
paul＠stjohnspoint.co.uk -
Reimar.Doeffinger＠stud.uni-karlsruhe.de