You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/18 14:18:05 UTC
[GitHub] [incubator-nuttx] yunkya2 opened a new pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
yunkya2 opened a new pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568
## Summary
The wav header parser in /dev/audio/pcm device driver expects the 'data' chunk is placed just after the 'fmt ' chunk.
Because the wav files generated by FFmpeg places 'LIST' chunk which contains the music track information between 'fmt ' and 'data' chunks, nxplayer cannot playback the files.
This patch skips extra chunks after 'fmt ' chunk to find the 'data' chunk.
## Impact
All architectures which support /dev/audio/pcm device.
## Testing
Tested by Raspberry Pi Pico with the upcoming audio driver.
nxplayer can playback the wav files which are created by FFmpeg after applying this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jerpelea commented on pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#issuecomment-823189033
@yunkya2 thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yunkya2 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
yunkya2 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615593835
##########
File path: audio/pcm_decode.c
##########
@@ -302,6 +303,25 @@ static uint16_t pcm_leuint32(uint32_t value)
}
#endif
+/****************************************************************************
+ * Name: pcm_leuint32_unaligned
+ *
+ * Description:
+ * Get a 32-bit value stored in little endian order. Unaligned address is
+ * acceptable.
+ *
+ ****************************************************************************/
+
+static uint32_t pcm_leuint32_unaligned(FAR const uint32_t *ptr)
Review comment:
I see. I have modified the patch to support unaligned access in both pcm_leuint32() and pcm_leuint16().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yunkya2 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
yunkya2 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615487435
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
BTW, I found another problems:
- The size of the 'LIST' chunk seems not always word-aligned. If not word-aligned 'LIST' chunk exists before 'data' chunk, getting the chunk ID and length may fails in the architectures which don't support unaligned word access such as Cortex-M0+.
- pcm_leuint32() implementation seems wrong. I think it doesn't work with big-endian architectures.
I'll also fix these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yunkya2 commented on pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
yunkya2 commented on pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#issuecomment-822241415
@jerpelea I got it and updated the patch. Is it ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#issuecomment-822239735
The change look good to me, please apply @jerpelea 's suggestion, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615558789
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
Sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yunkya2 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
yunkya2 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615485533
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
The condition `len < ret` is ok because:
- The initial value of `ret` (= `sizeof(struct wav_header_s)`) already contains the size of `struct wav_datachunk_s`.
In the case of no extra chunk, we can just compare with `ret` and the length of the incoming data `len`.
- The case of `len == ret` is invalid because it means the incoming data only have wav header and there is no data body to be transferred.
But, your `for` loop seems better because we don't need to compare the values twice. I'd like to modify the loop to yours except the error condition check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615560663
##########
File path: audio/pcm_decode.c
##########
@@ -302,6 +303,25 @@ static uint16_t pcm_leuint32(uint32_t value)
}
#endif
+/****************************************************************************
+ * Name: pcm_leuint32_unaligned
+ *
+ * Description:
+ * Get a 32-bit value stored in little endian order. Unaligned address is
+ * acceptable.
+ *
+ ****************************************************************************/
+
+static uint32_t pcm_leuint32_unaligned(FAR const uint32_t *ptr)
Review comment:
should we call pcm_leuint32_unaligned(rename to pcm_leuint32) in all place? I am afraid that the alignment happen in other place too. pcm_leuint16 may have the same problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#issuecomment-822281586
I think it's good now, let's merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615427880
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
or change to:
```
for (; ; )
{
localwav.data.chunkid = pcm_leuint32(dchunk->chunkid);
localwav.data.chunklen = pcm_leuint32(dchunk->chunklen);
if (localwav.data.chunkid == WAV_DATA_CHUNKID)
{
break;
}
/* Not data chunk. Skip it. */
ret += localwav.data.chunklen + 8;
if (ret +8 >= len)
{
return -EINVAL;
}
dchunk = (FAR const struct wav_datachunk_s *)
((uintptr_t)dchunk + localwav.data.chunklen + 8);
}
```
And remove line 387-392
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615427453
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
should the condition is ret+ 8 <= len?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3568: audio/pcm_decode.c: skip extra chunk of wav header
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3568:
URL: https://github.com/apache/incubator-nuttx/pull/3568#discussion_r615427880
##########
File path: audio/pcm_decode.c
##########
@@ -356,17 +363,45 @@ static bool pcm_parsewav(FAR struct pcm_decode_s *priv, uint8_t *data)
localwav.fmt.align = pcm_leuint16(wav->fmt.align);
localwav.fmt.bpsamp = pcm_leuint16(wav->fmt.bpsamp);
- localwav.data.chunkid = pcm_leuint32(wav->data.chunkid);
- localwav.data.chunklen = pcm_leuint32(wav->data.chunklen);
+ /* Find the data chunk */
+
+ dchunk = &wav->data;
+
+ while (len < ret)
Review comment:
or change to:
```
for (; ; )
{
localwav.data.chunkid = pcm_leuint32(dchunk->chunkid);
localwav.data.chunklen = pcm_leuint32(dchunk->chunklen);
if (localwav.data.chunkid == WAV_DATA_CHUNKID)
{
break;
}
/* Not data chunk. Skip it. */
ret += localwav.data.chunklen + 8;
if (ret + 8 >= len)
{
return -EINVAL;
}
dchunk = (FAR const struct wav_datachunk_s *)
((uintptr_t)dchunk + localwav.data.chunklen + 8);
}
```
And remove line 387-392
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org