[PATCH] - fix for audio and video in dvr-ms asf files
Hi I've been doing some work on the asf demuxer in ffmpeg and apparently there's some interest in making the mplayer demuxer work with dvr-ms files too. So I've attached a patch that fixes most of the audio and video problems that occur when attempting to play dvr-ms in mplayer. The audio fix is a port of the work I did in ffmpeg. The way I've done it here is a little different though... if the dvr-ms guid is discovered in the header then some special case code is executed to initialise the audio. This shouldn't affect regular asf files and has minimal impact on the existing code. The video fix is basically a lift of the work done by others on the ffmpeg asf demuxer. The reason dvr-ms files haven't worked until now is that the mpeg2 video frames need to be parsed out of the data segments. The existing code assumes that when the media object number changes then you've got your frame. This is not the case with dvr-ms files though. FFmpeg has a generic way of handling this kind of parsing and there may well be some equivalent in mplayer. I haven't had time to look though so I've just taken the relevant bits of code from ffmpeg and dropped them into demux_asf.c. I dont know if it's acceptable to do that but I'm sure you'll let me know. If there's a better way to do it please point me in the right direction and I'll have a go at some point. There are still at least the following issues to be resolved that I hope to get to at some point... seeking is broken, the frame height is wrong, the fps and bit rate displayed at the start is wrong and there's no support for subtitles. If you're going to tackle these, please let me know so as we don't duplicate effort. Regards, John
John Donaghy wrote:
There are still at least the following issues to be resolved that I hope to get to at some point... seeking is broken, the frame height is wrong, the fps and bit rate displayed at the start is wrong and there's no support for subtitles. If you're going to tackle these, please let me know so as we don't duplicate effort.
Regards,
John
// create new packet: Index: demuxer.h =================================================================== RCS file: /cvsroot/mplayer/main/libmpdemux/demuxer.h,v retrieving revision 1.91 diff -u -r1.91 demuxer.h --- demuxer.h 17 Feb 2006 01:57:41 -0000 1.91 +++ demuxer.h 31 Mar 2006 17:35:25 -0000 @@ -110,6 +110,9 @@ // ---- asf ----- demux_packet_t *asf_packet; // read asf fragments here int asf_seq; + int asf_is_dvr_ms; + uint32_t asf_frame_state; + int asf_frame_start_found; // ---- mov ----- unsigned int ss_mul,ss_div; // ---- stream header ----
although I'm not the maintener of demux_asf I can tell you that the above additions to demuxer.h (in demux_stream_t) are not acceptable (asf_seq shoule be removed, too). Please, find another way: possibly make them members of demuxer->priv
although I'm not the maintener of demux_asf I can tell you that the above additions to demuxer.h (in demux_stream_t) are not acceptable (asf_seq shoule be removed, too). Please, find another way: possibly make them members of demuxer->priv
It's a fair point. I've followed your suggestion wrt my three additions.
Any word on whether or not this patch for dvr-ms support is going to be accepted? I submitted the change requested by Nico over a week ago but still havent heard anything. On 3/31/06, John Donaghy <johnfdonaghy@gmail.com> wrote:
although I'm not the maintener of demux_asf I can tell you that the above additions to demuxer.h (in demux_stream_t) are not acceptable (asf_seq shoule be removed, too). Please, find another way: possibly make them members of demuxer->priv
It's a fair point. I've followed your suggestion wrt my three additions.
On Sunday, 09 April 2006 at 16:40, John Donaghy wrote:
Any word on whether or not this patch for dvr-ms support is going to be accepted? I submitted the change requested by Nico over a week ago but still havent heard anything.
Please be patient. We're all very busy and reviewing takes some time. Rest assured, your patch has not been lost or overlooked. Regards, R. -- MPlayer developer and RPMs maintainer: http://rpm.greysector.net/mplayer/ There should be a science of discontent. People need hard times and oppression to develop psychic muscles. -- from "Collected Sayings of Muad'Dib" by the Princess Irulan
On Fri, Mar 31, 2006 at 02:28:32PM -0600, John Donaghy wrote:
although I'm not the maintener of demux_asf I can tell you that the above additions to demuxer.h (in demux_stream_t) are not acceptable (asf_seq shoule be removed, too). Please, find another way: possibly make them members of demuxer->priv
It's a fair point. I've followed your suggestion wrt my three additions.
This crashes in demux_open for me on my G4: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500448 (LWP 9422)] 0x0ecc79e4 in memcmp () from /lib/tls/libc.so.6 Let me know if you need more details. Diego
This crashes in demux_open for me on my G4:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500448 (LWP 9422)] 0x0ecc79e4 in memcmp () from /lib/tls/libc.so.6
Let me know if you need more details.
Anything you can provide would be helpful. Did you try it on x86? Also, sorry to sound like a newbie but did you merge the patch using a CSV snap from the day it was submitted or on a current snapshot? Does it work with '-demurer 35'? Can you get the file you used to me somehow? Was it a dvr-ms file generated from Windows Media Center 2005 Rollup 2 or something earlier. I know some things are different in recent versions - such as the presence of a subtitle stream - which could affect things. I had a look at the patch and the only memcmp I've added is in this function: static int find_backwards_asf_guid(char *buf, const char *guid, int cur_pos) { int i; for (i=cur_pos; i>0; i--) { if (memcmp(&buf[i], guid, 16) == 0) return i + 16 + 8; // point after guid + length } return -1; } I cant immediately see anything wrong with it except that maybe it should start at cur_pos - 16 instead of cur_pos ie: static int find_backwards_asf_guid(char *buf, const char *guid, int cur_pos) { int i; for (i=cur_pos-16; i>0; i--) { if (memcmp(&buf[i], guid, 16) == 0) return i + 16 + 8; // point after guid + length } return -1; } it's very similar to the existing find_asf_guid... int find_asf_guid(char *buf, const char *guid, int cur_pos, int buf_len) { int i; for (i = cur_pos; i < buf_len - 19; i++) { if (memcmp(&buf[i], guid, 16) == 0) return i + 16 + 8; // point after guid + length } return -1; } Regards, John
This crashes in demux_open for me on my G4:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500448 (LWP 9422)] 0x0ecc79e4 in memcmp () from /lib/tls/libc.so.6
Let me know if you need more details.
Anything you can provide would be helpful. Did you try it on x86?
Diego I dont know how to progress this any further. The patch seems to work fine on Windows and I could try it on Linux (x86) just to be sure if you think it's worth it. However I dont have access to a G4 machine so I'm not sure what I can do. Do you want to discuss the patch with me to see if we can figure out what might be causing the problem? John
On Tue, Apr 25, 2006 at 10:55:48AM -0500, John Donaghy wrote:
This crashes in demux_open for me on my G4:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500448 (LWP 9422)] 0x0ecc79e4 in memcmp () from /lib/tls/libc.so.6
Let me know if you need more details.
Anything you can provide would be helpful. Did you try it on x86?
Diego
I dont know how to progress this any further. The patch seems to work fine on Windows and I could try it on Linux (x86) just to be sure if you think it's worth it. However I dont have access to a G4 machine so I'm not sure what I can do.
Do you want to discuss the patch with me to see if we can figure out what might be causing the problem?
I'll investigate this further in the next few days. I'm rather busy, that's why it slipped off my radar, but I haven't forgotten about it. Thanks for following up. Diego
On Tue, Apr 25, 2006 at 07:17:34PM +0200, Diego Biurrun wrote:
On Tue, Apr 25, 2006 at 10:55:48AM -0500, John Donaghy wrote:
This crashes in demux_open for me on my G4:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500448 (LWP 9422)] 0x0ecc79e4 in memcmp () from /lib/tls/libc.so.6
Let me know if you need more details.
Anything you can provide would be helpful. Did you try it on x86?
Diego
I dont know how to progress this any further. The patch seems to work fine on Windows and I could try it on Linux (x86) just to be sure if you think it's worth it. However I dont have access to a G4 machine so I'm not sure what I can do.
Do you want to discuss the patch with me to see if we can figure out what might be causing the problem?
I'll investigate this further in the next few days. I'm rather busy, that's why it slipped off my radar, but I haven't forgotten about it. Thanks for following up.
OK, I teste on my x86 box, it works fine there, but immediately crashes on my G4. Here is a backtrace: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500416 (LWP 28423)] 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 (gdb) bt #0 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 #1 0x10199534 in find_asf_guid (buf=<value optimized out>, guid=0x104c5db0 "\221\aÜ··©Ï\021\216æ", cur_pos=<value optimized out>, buf_len=<value optimized out>) at asfheader.c:153 #2 0x10199e08 in read_asf_header (demuxer=0x10737918, asf=0x107365c0) at asfheader.c:258 #3 0x1013820c in demux_open_asf (demuxer=0x9873a2dc) at demux_asf.c:563 #4 0x101354a8 in demux_open_stream (stream=0x200, file_format=-2013257395, force=0, audio_id=0, video_id=0, dvdsub_id=276013735, filename=0x0) at demuxer.c:770 #5 0x10135794 in demux_open (vs=0x10736f80, file_format=273440176, audio_id=9393, video_id=-1, dvdsub_id=0, filename=0x107267a0 "/home/diego/tmp/bug_reports/demuxer/microsoft-new-way-to-shove-mpeg2-in-asf.dvr-ms") at demuxer.c:844 #6 0x1004d1d0 in main (argc=<value optimized out>, argv=<value optimized out>) at mplayer.c:3032 the sample is at http://www.mplayerhq.hu/MPlayer/samples/dvr_ms/microsoft-new-way-to-shove-mp... Let me know if I can provide further information. Diego
OK, I teste on my x86 box, it works fine there, but immediately crashes on my G4. Here is a backtrace:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500416 (LWP 28423)] 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 (gdb) bt #0 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 #1 0x10199534 in find_asf_guid (buf=<value optimized out>, guid=0x104c5db0 "\221\aÜ··(c)Ï\021\216æ", cur_pos=<value optimized out>, buf_len=<value optimized out>) at asfheader.c:153 #2 0x10199e08 in read_asf_header (demuxer=0x10737918, asf=0x107365c0) at asfheader.c:258 #3 0x1013820c in demux_open_asf (demuxer=0x9873a2dc) at demux_asf.c:563
Thanks for that. This is difficult since I dont have a Mac but it looks like it's crashing in a function that I didnt change. Can you see what happens if you try to play this file without my patch applied? I'm assuming it'll work but you never know. If it does work without the patch then the only way forward that I can see is for me to send you a debug version to try to narrow down the cause. Do you have time to work with me on that? John
On Wed, Apr 26, 2006 at 10:05:54AM -0500, John Donaghy wrote:
OK, I teste on my x86 box, it works fine there, but immediately crashes on my G4. Here is a backtrace:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 805500416 (LWP 28423)] 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 (gdb) bt #0 0x0eca09e4 in memcmp () from /lib/tls/libc.so.6 #1 0x10199534 in find_asf_guid (buf=<value optimized out>, guid=0x104c5db0 "\221\aÜ··(c)Ï\021\216æ", cur_pos=<value optimized out>, buf_len=<value optimized out>) at asfheader.c:153 #2 0x10199e08 in read_asf_header (demuxer=0x10737918, asf=0x107365c0) at asfheader.c:258 #3 0x1013820c in demux_open_asf (demuxer=0x9873a2dc) at demux_asf.c:563
Thanks for that. This is difficult since I dont have a Mac but it looks like it's crashing in a function that I didnt change. Can you see what happens if you try to play this file without my patch applied? I'm assuming it'll work but you never know.
It works, but no sound and visual artifacts. Same as on x86.
If it does work without the patch then the only way forward that I can see is for me to send you a debug version to try to narrow down the cause. Do you have time to work with me on that?
I'll make some time. Send me your debug patch. Diego
I'll make some time. Send me your debug patch.
Great, thanks. This version of the patch has a few extra 'verbose' messages that should better indicate where the problem lies. John
John Donaghy writes the following:
I'll make some time. Send me your debug patch.
Great, thanks. This version of the patch has a few extra 'verbose' messages that should better indicate where the problem lies.
OK, here's the problem: you find an asf_ext_stream_audio header, and do le2me_ASF_stream_header_t(streamh) on it (line 255 of asfheader.c after your patch). Later on, the same header is found again by the main header-finding loop, and le2me_ASF_stream_header_t is done on it again (line 275). Having been byte-swapped twice, it is now back in its original little-endian form. The type_size field which should be 0x88 (in the sample file at least) is instead seen as 0x88000000 which is not good, and it's signed which makes it double plus ungood. You need to find a way to ensure that le2me_ASF_stream_header_t doesn't get called twice on the same header.
OK, here's the problem: you find an asf_ext_stream_audio header, and do le2me_ASF_stream_header_t(streamh) on it (line 255 of asfheader.c after your patch). Later on, the same header is found again by the main header-finding loop, and le2me_ASF_stream_header_t is done on it again (line 275). Having been byte-swapped twice, it is now back in its original little-endian form. The type_size field which should be 0x88 (in the sample file at least) is instead seen as 0x88000000 which is not good, and it's signed which makes it double plus ungood.
You need to find a way to ensure that le2me_ASF_stream_header_t doesn't get called twice on the same header.
Excellent! Thanks for spotting that. It'd probably have taken me a very long time to see that - even if I had a mac! The attached version should avoid this problem by starting the main header search loop after the position of the previously detected audio stream if there is one. I've left a couple of extra debug messages in (that give compiler warnings) in case it doesn't work. If Diego or someone else can confirm that this now works on a mac I can take those messages out and resubmit. Thanks, John
On Thu, Apr 27, 2006 at 11:32:55AM -0500, John Donaghy wrote:
The attached version should avoid this problem by starting the main header search loop after the position of the previously detected audio stream if there is one. I've left a couple of extra debug messages in (that give compiler warnings) in case it doesn't work. If Diego or someone else can confirm that this now works on a mac I can take those messages out and resubmit.
Success! :) Diego
Hi, On Thu, Apr 27, 2006 at 01:20:22PM -0500, John Donaghy wrote:
Success! :)
Thanks for testing it. Hopefully this version will be acceptable.
I can't comment much on the correctness, but it looks pretty clean. A few small nits:
+static int find_backwards_asf_guid(char *buf, const char *guid, int cur_pos) +{ + int i; + for (i=cur_pos; i>0; i--) { + if (memcmp(&buf[i], guid, 16) == 0) + return i + 16 + 8; // point after guid + length + } + return -1; +} +
I wonder if either a buf_len parameter or starting from cur_pos - 16 might be a good idea, but I guess you know better. But this reminds reminds me: please add doxygen comments as described in DOCS/tech/code-documentation.txt, so at least assumptions like "cur_pos point at most to a position 16 bytes before the end of the buffer" are documentation, because otherwise I'm sure somebody will sooner or later miss that little detail.
@@ -156,7 +204,10 @@ int best_video = -1; int best_audio = -1; uint64_t data_len; - + ASF_stream_header_t *streamh; + uint8_t *buffer; + int audio_pos=0; + if(hdr_len < 0) {
cosmetics (and a bad one at that *g*), you added trailing whitespace to the empty line.
- { - ASF_stream_header_t *streamh = (ASF_stream_header_t *)&hdr[pos]; - uint8_t *buffer; + { + streamh = (ASF_stream_header_t *)&hdr[pos];
cosmetics as well, what did you change in that { line?
@@ -30,6 +35,28 @@ ((unsigned char*)(p))[1]<<8) #endif
+#if defined(ARCH_X86) || defined(ARCH_X86_64) +# define unaligned32(a) (*(const uint32_t*)(a)) +#else +# ifdef __GNUC__ +static inline uint32_t unaligned32(const void *v) { + struct Unaligned { + uint32_t i; + } __attribute__((packed)); + + return ((const struct Unaligned *) v)->i; +} +# elif defined(__DECC) +static inline uint32_t unaligned32(const void *v) { + return *(const __unaligned uint32_t *) v; +} +# else +static inline uint32_t unaligned32(const void *v) { + return *(const uint32_t *) v; +} +# endif +#endif //!ARCH_X86 +
Ugh. Scary. Why can't you use the LOAD_LE32 macros? Or alternatively make similar LOAD_BE32 macros? This at least looks like both a portability and maintainability macros - at best acceptable in highly performance-critical code (if it makes a performance difference at all).
@@ -105,16 +225,10 @@ ds_add_packet(ds,ds->asf_packet); ds->asf_packet=NULL; } else { - // append data to it! - demux_packet_t* dp=ds->asf_packet; - if(dp->len!=offs && offs!=-1) mp_msg(MSGT_DEMUX,MSGL_V,"warning! fragment.len=%d BUT next fragment offset=%d \n",dp->len,offs); - dp->buffer=realloc(dp->buffer,dp->len+len+FF_INPUT_BUFFER_PADDING_SIZE); - memcpy(dp->buffer+dp->len,data,len); - memset(dp->buffer+dp->len+len, 0, FF_INPUT_BUFFER_PADDING_SIZE); - mp_dbg(MSGT_DEMUX,MSGL_DBG4,"data appended! %d+%d\n",dp->len,len); - dp->len+=len; - // we are ready now. - return 1; + // append data to it! + demux_asf_append_to_packet(dp,data,len,offs); + // we are ready now. + return 1;
Some cosmetics here as well. Greetings, Reimar Döffinger
Hi Thanks for reviewing my changes
+static int find_backwards_asf_guid(char *buf, const char *guid, int cur_pos) +{ + int i; + for (i=cur_pos; i>0; i--) { + if (memcmp(&buf[i], guid, 16) == 0) + return i + 16 + 8; // point after guid + length + } + return -1; +} +
I wonder if either a buf_len parameter or starting from cur_pos - 16 might be a good idea, but I guess you know better. But this reminds reminds me: please add doxygen comments as described in DOCS/tech/code-documentation.txt, so at least assumptions like "cur_pos point at most to a position 16 bytes before the end of the buffer" are documentation, because otherwise I'm sure somebody will sooner or later miss that little detail.
I agree it's better to start at cur_pos - 16 so I'll add that to the next submission. In that case do you still want me to document the new function (and can you give me an example of how this should look)?
cosmetics (and a bad one at that *g*), you added trailing whitespace to the empty line.
cosmetics as well, what did you change in that { line? Not sure how the trailing space got there. I will deal with these too.
@@ -30,6 +35,28 @@ ((unsigned char*)(p))[1]<<8) #endif
+#if defined(ARCH_X86) || defined(ARCH_X86_64) +# define unaligned32(a) (*(const uint32_t*)(a)) +#else +# ifdef __GNUC__ +static inline uint32_t unaligned32(const void *v) { + struct Unaligned { + uint32_t i; + } __attribute__((packed)); + + return ((const struct Unaligned *) v)->i; +} +# elif defined(__DECC) +static inline uint32_t unaligned32(const void *v) { + return *(const __unaligned uint32_t *) v; +} +# else +static inline uint32_t unaligned32(const void *v) { + return *(const uint32_t *) v; +} +# endif +#endif //!ARCH_X86 +
Ugh. Scary. Why can't you use the LOAD_LE32 macros? Or alternatively make similar LOAD_BE32 macros? This at least looks like both a portability and maintainability macros - at best acceptable in highly performance-critical code (if it makes a performance difference at all).
This piece of code is pretty much copied from ffmpeg. I didn't change it because I'm not sure I fully understand the implications of doing so on all the different architectures. There is one call to 'unaligned32' in the 'find_start_code' function (which itself originates in ffmpeg): *state= be2me_32(unaligned32(p)); If you can tell me how I could do it with the MPlayer macros in an 'architecture-safe' way I'd be grateful. Also, what about the ASFMIN macro I added? - is there one already defined somewhere else that I can pick up (sorry, but I don't know the mplayer code at all well) Regards, John
+#if defined(ARCH_X86) || defined(ARCH_X86_64) +# define unaligned32(a) (*(const uint32_t*)(a)) +#else +# ifdef __GNUC__ +static inline uint32_t unaligned32(const void *v) { + struct Unaligned { + uint32_t i; + } __attribute__((packed)); + + return ((const struct Unaligned *) v)->i; +} +# elif defined(__DECC) +static inline uint32_t unaligned32(const void *v) { + return *(const __unaligned uint32_t *) v; +} +# else +static inline uint32_t unaligned32(const void *v) { + return *(const uint32_t *) v; +} +# endif +#endif //!ARCH_X86 +
Ugh. Scary. Why can't you use the LOAD_LE32 macros? Or alternatively make similar LOAD_BE32 macros? This at least looks like both a portability and maintainability macros - at best acceptable in highly performance-critical code (if it makes a performance difference at all).
I've replaced this in the attached version with a LOAD_BE32 macro which I think is what you're suggesting but I'm not sure. It appears to work on x86 but I'm not very knowlegable in this area. Can you review and let me know if this is a correct and acceptable approach. Thanks, John
On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
I've replaced this in the attached version with a LOAD_BE32 macro which I think is what you're suggesting but I'm not sure. It appears to work on x86 but I'm not very knowlegable in this area. Can you review and let me know if this is a correct and acceptable approach.
I've just tested this, it works fine on big-endian. Diego
Diego Biurrun writes the following:
On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
I've replaced this in the attached version with a LOAD_BE32 macro which I think is what you're suggesting but I'm not sure. It appears to work on x86 but I'm not very knowlegable in this area. Can you review and let me know if this is a correct and acceptable approach.
I've just tested this, it works fine on big-endian.
I was just moving uncommitted changes from my CVS tree to an SVN tree, and found this dvr_ms code still lingering there. Since the last word on the subject was "works fine", was someone going to commit it?
2006/6/6, Alan Curry <pacman@theworld.com>:
Diego Biurrun writes the following:
On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
I've replaced this in the attached version with a LOAD_BE32 macro which I think is what you're suggesting but I'm not sure. It appears to work on x86 but I'm not very knowlegable in this area. Can you review and let me know if this is a correct and acceptable approach.
I've just tested this, it works fine on big-endian.
I was just moving uncommitted changes from my CVS tree to an SVN tree, and found this dvr_ms code still lingering there. Since the last word on the subject was "works fine", was someone going to commit it?
You are free to do it.
Hi, On Tue, Jun 06, 2006 at 12:10:27PM +0300, Ivan Kalvachev wrote:
2006/6/6, Alan Curry <pacman@theworld.com>:
I was just moving uncommitted changes from my CVS tree to an SVN tree, and found this dvr_ms code still lingering there. Since the last word on the subject was "works fine", was someone going to commit it?
You are free to do it.
Just make sure you have the latest version, from May 18 I think. I didn't see anything objectionable in a quick look... Greetings, Reimar Döffinger
Reimar =?iso-8859-1?Q?D=F6ffinger?= writes the following:
Hi, On Tue, Jun 06, 2006 at 12:10:27PM +0300, Ivan Kalvachev wrote:
2006/6/6, Alan Curry <pacman@theworld.com>:
I was just moving uncommitted changes from my CVS tree to an SVN tree, and found this dvr_ms code still lingering there. Since the last word on the subject was "works fine", was someone going to commit it?
You are free to do it.
Just make sure you have the latest version, from May 18 I think. I didn't see anything objectionable in a quick look...
I committed the last version I know of.
Ivan Kalvachev writes the following:
You are free to do it.
I'm still waiting for that thread about the svn passwords to produce usable information. Until then, no committing.
Alan Curry wrote:
Ivan Kalvachev writes the following:
You are free to do it.
I'm still waiting for that thread about the svn passwords to produce usable information. Until then, no committing.
If you haven't heard it elsewhere yet: Just email a username and password to Diego. For continuity, the username should be the same one you used with CVS. -Corey
On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
+#if defined(ARCH_X86) || defined(ARCH_X86_64) +# define unaligned32(a) (*(const uint32_t*)(a)) +#else +# ifdef __GNUC__ +static inline uint32_t unaligned32(const void *v) { + struct Unaligned { + uint32_t i; + } __attribute__((packed)); + + return ((const struct Unaligned *) v)->i; +} +# elif defined(__DECC) +static inline uint32_t unaligned32(const void *v) { + return *(const __unaligned uint32_t *) v; +} +# else +static inline uint32_t unaligned32(const void *v) { + return *(const uint32_t *) v; +} +# endif +#endif //!ARCH_X86 +
Ugh. Scary. Why can't you use the LOAD_LE32 macros? Or alternatively make similar LOAD_BE32 macros? This at least looks like both a portability and maintainability macros - at best acceptable in highly performance-critical code (if it makes a performance difference at all).
I've replaced this in the attached version with a LOAD_BE32 macro which I think is what you're suggesting but I'm not sure. It appears to work on x86 but I'm not very knowlegable in this area. Can you review and let me know if this is a correct and acceptable approach.
Reimar, are you OK with the patch now? I can confirm that it works on PPC. It would be nice to have this in pre8. Diego
Hi, On Thu, May 18, 2006 at 01:18:29AM +0200, Diego Biurrun wrote:
Reimar, are you OK with the patch now? I can confirm that it works on PPC. It would be nice to have this in pre8.
If someone tested on several asf files and esp. streams I won't object. Greetings, Reimar Döffinger
This pos=0 line is pointless, get rid of it.
Here's the patch again with the offending line removed.
Hi, On Thu, May 18, 2006 at 10:03:20AM -0500, John Donaghy wrote:
This pos=0 line is pointless, get rid of it.
Here's the patch again with the offending line removed.
Thanks, but just to clarify: this was actually meant as a comment to the (potential) committer, I didn't intend to ask for a new patch because of such a trivial and irrelevant change :-) Greetings, Reimar Döffinger
Thanks, but just to clarify: this was actually meant as a comment to the (potential) committer, I didn't intend to ask for a new patch because of such a trivial and irrelevant change :-)
Good to know for future reference. I assumed you were the committer. I havent done this much so I dont really know who does what.
Hi, I can't see any obvious problems with this patch, though I didn't check too much. Just one thing: On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
+ pos=0; + if ((pos = find_asf_guid(hdr, asf_ext_stream_audio, pos, hdr_len)) >= 0) + {
This pos=0 line is pointless, get rid of it. Greetings, Reimar Döffinger
Hi The patch I submitted to enable mplayer to play dvr-ms files was eventually committed some time ago after a lengthy review process. Unfortunately, however, a rather important line of code was removed during the review that has the effect of making the audio stream undetectable.
I can't see any obvious problems with this patch, though I didn't check too much. Just one thing:
On Sat, Apr 29, 2006 at 05:45:44PM -0500, John Donaghy wrote:
+ pos=0; + if ((pos = find_asf_guid(hdr, asf_ext_stream_audio, pos, hdr_len)) >= 0) + {
This pos=0 line is pointless, get rid of it.
It turns out that this line of code is not pointless but in fact essential because otherwise the pos variable is used before it is initialised. At the time, I resubmitted the patch without the line in question based on Reimar's comment, and I assume that I checked that it worked at the time. Perhaps the uninitialised variable just happened to contain zero at the time I tested it. (I was certainly bored with the process by this point so I didn't think too hard about it!) Anyway, I'm requesting that this "pos=0" line be added back into asf_header,line 233. Should I submit a patch or is the information I've provided here enough to enable the change to be made? Regards, John Donaghy
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
On Mon, 2006-07-17 at 16:45 -0500, John Donaghy wrote:
This pos=0 line is pointless, get rid of it.
It turns out that this line of code is not pointless but in fact essential because otherwise the pos variable is used before it is initialised. At the time, I resubmitted the patch without the line in question based on Reimar's comment, and I assume that I checked that it worked at the time. Perhaps the uninitialised variable just happened to contain zero at the time I tested it. (I was certainly bored with the process by this point so I didn't think too hard about it!)
Anyway, I'm requesting that this "pos=0" line be added back into asf_header,line 233. Should I submit a patch or is the information I've provided here enough to enable the change to be made?
I'll change the function call to use 0 as the parameter instead of pos.
On Fri, Mar 31, 2006 at 12:07:05PM -0600, John Donaghy wrote:
I've been doing some work on the asf demuxer in ffmpeg and apparently [...]
Say, you've learned a bit about the ASF format, could you possibly document you knowledge in the multimedia wiki? The entry about ASF could use some love and especially all the dvr-ms issues are completely undocumented AFAIK. http://wiki.multimedia.cx/index.php?title=Microsoft_Advanced_Streaming_Forma... Diego
Say, you've learned a bit about the ASF format, could you possibly document you knowledge in the multimedia wiki? The entry about ASF could use some love and especially all the dvr-ms issues are completely undocumented AFAIK.
I can certainly add what I know about dvr-ms now, and I can flesh out the general ASF information a bit as well. But I know very little about other container formats so I wont be able to say much about how good ASF is for example. The Wikipedia entry on ASF is pretty good though. Can we use any of it or does it have to be original? Also, is there a specific forum (other than this one) to discuss questions such as this one? Regarding the patch I submitted, any word on whether or not it is acceptable? I'm expecting at least some change requests apart from Nico's but I havent heard anything yet. John
John Donaghy wrote:
Say, you've learned a bit about the ASF format, could you possibly document you knowledge in the multimedia wiki? The entry about ASF could use some love and especially all the dvr-ms issues are completely undocumented AFAIK.
I can certainly add what I know about dvr-ms now, and I can flesh out the general ASF information a bit as well. But I know very little about other container formats so I wont be able to say much about how good ASF is for example. The Wikipedia entry on ASF is pretty good though. Can we use any of it or does it have to be original?
As long as you don't violate a copyright, there should be no problem with using information from anywhere else. You should probably reference the original source as a courtesy and because it could be updated at any time.
Also, is there a specific forum (other than this one) to discuss questions such as this one?
The ffmpeg-devel mailing list is more or less the definitive place; much of mplayer's codec- and container-related code is in ffmpeg (via libavcodec and libavformat), and occasionally an effort is made to move something useful from mplayer to ffmpeg so it can be re-used by other projects and be implemented with less code duplication.
Regarding the patch I submitted, any word on whether or not it is acceptable? I'm expecting at least some change requests apart from Nico's but I havent heard anything yet.
I can't comment on that, though. -Corey
Say, you've learned a bit about the ASF format, could you possibly document you knowledge in the multimedia wiki? The entry about ASF could use some love and especially all the dvr-ms issues are completely undocumented AFAIK.
http://wiki.multimedia.cx/index.php?title=Microsoft_Advanced_Streaming_Forma...
I'm been a bit busy lately so I havent had much time for this but I have now added some basic text to the ASF section and also created a dvr-ms page. I hope to tidy these up and expand them a bit at some point. Feel free to review and generally tear them apart though. John
On Tue, May 02, 2006 at 03:22:50PM -0500, John Donaghy wrote:
Say, you've learned a bit about the ASF format, could you possibly document you knowledge in the multimedia wiki? The entry about ASF could use some love and especially all the dvr-ms issues are completely undocumented AFAIK.
http://wiki.multimedia.cx/index.php?title=Microsoft_Advanced_Streaming_Forma...
I'm been a bit busy lately so I havent had much time for this but I have now added some basic text to the ASF section and also created a dvr-ms page. I hope to tidy these up and expand them a bit at some point. Feel free to review and generally tear them apart though.
Nice to see you working on this. Now what is HTTV? HDTV? Diego
Nice to see you working on this. Now what is HTTV? HDTV?
Didnt you know that's the new name for it here in America... Horribly Trashy Television.
participants (9)
-
Alan Curry -
Corey Hickey -
Diego Biurrun -
Dominik 'Rathann' Mierzejewski -
Ivan Kalvachev -
John Donaghy -
Nico Sabbi -
Reimar Döffinger -
Uoti Urpala