GNOME Bugzilla – Bug 752613
qtdemux: raw 16 bit PCM audio in 'raw ' fourcc not working
Last modified: 2015-08-16 13:38:59 UTC
Created attachment 307730 [details] This is the test file to be used to reproduce bug Overview: By Default, unsigned 8-bit raw PCM caps are always set and unsigned 16-bit data is stored in little endian format. For unsigned raw PCM data caps are not set according to actual bits per sample. Steps to Reproduce: 1) Play the attached MOV file. Actual Results: Audio Noise is heard. Expected Results: Expectation is to play audio without noise. Build Date & Hardware: Build 2014-12-18 on Ubuntu 12.04
Created attachment 307731 [details] [review] The above is the diff patch code.
Did you attach the right test file? The test file contains AAC, not PCM audio.
(In reply to Tim-Philipp Müller from comment #2) > Did you attach the right test file? > > The test file contains AAC, not PCM audio. Sorry,for wrong attachment.will attach right file now.
My file size is 2234KB ,unable to upload. Can u please let me know how to upload large files?
Perhaps upload it to a file hosting website, or some personal/company web space. Or split it into multiple files with 'split -b 1M file.mp4' and attach the file parts (but then please also provide the md5sum or sha1sum of the original file).
Feel free to just send me the file by e-mail if it's just ~2MB.
Created attachment 307955 [details] This is split file part whose md5 sum is e476e8c5f12498d1dff4735732900aa5
Created attachment 307956 [details] This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5
Created attachment 307957 [details] This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5
Thanks Tim, I have attached files by splitting them with there md5sum.
Comment on attachment 307731 [details] [review] The above is the diff patch code. Thanks for the patch and the sample files. I can confirm the patch works. Please could you update the patch as follows: - provide a patch in 'git format-patch' format with author and commit message set and bugzilla url to this ticket in the commit message - there seem to be spaces and/or tabs at the end of some lines (they show up in red when you do 'git diff' for uncommitted changes or 'git show' for the last commit). Please remove those. - please ensure correct indentation (spaces only, no tabs please). The easiest way is to run gst-indent on the code (but see next item) - I think in the NONE/raw case, we should just check for stream->bytes_per_packet == 1 and use the current code, and otherwise just fall through to the 'twos' case, which does what we want already.
Tim,Did you mean that, I should get developer access for the patch and submit the changes?
Created attachment 308050 [details] [review] This is the diff-patch.
The following is the case observed: 1. In NONE/raw case the code is not checking the type of endian, which is simply set to Little endian format. 2. The attached file has 16-bit PCM data which is being set to 8-bit data by default. 3. stream->bytes_per_packet might not be always 1 in NONE/raw case.
> The following is the case observed: > > 1. In NONE/raw case the code is not checking the type of endian, which is > simply set to Little endian format. I don't understand what you mean by this. With the current code, no endianness is set for NONE/raw, and it's not needed since it's only U8. With your patch it's set to BIG_ENDIAN, which is correct according to the quicktime specs. With my suggestion to fallthrough to the 'twos' case, it would also be set to big endian. > 2. The attached file has 16-bit PCM data which is being set to 8-bit data by > default. I understand this. And your patch fixes it. I just asked you to fix it differently, with less code. > 3. stream->bytes_per_packet might not be always 1 in NONE/raw case. I understand this. And your patch fixes it. I just asked you to fix it differently, with less code. Also: 4. In the 'twos' case, the data might be 8-bit (S8 in this case) as well. The code currently doesn't handle that. I think the NONE/raw/twos case could be unified into one block.
(In reply to Tim-Philipp Müller from comment #15) > > The following is the case observed: > > > > 1. In NONE/raw case the code is not checking the type of endian, which is > > simply set to Little endian format. > > I don't understand what you mean by this. With the current code, no > endianness is set for NONE/raw, and it's not needed since it's only U8. With > your patch it's set to BIG_ENDIAN, which is correct according to the > quicktime specs. With my suggestion to fallthrough to the 'twos' case, it > would also be set to big endian. > Since initially break statement is place after NONE/raw, case was not going to twos case so we placed code again in NONE/raw. > > > 2. The attached file has 16-bit PCM data which is being set to 8-bit data by > > default. > > I understand this. And your patch fixes it. I just asked you to fix it > differently, with less code. > Yes, Will try to fix it in less code. > > > 3. stream->bytes_per_packet might not be always 1 in NONE/raw case. > > I understand this. And your patch fixes it. I just asked you to fix it > differently, with less code. > > Also: > > 4. In the 'twos' case, the data might be 8-bit (S8 in this case) as well. > The code currently doesn't handle that. > > I think the NONE/raw/twos case could be unified into one block. I agree to this idea of unifying. Also: I will try to unify and submit the same.
Created attachment 308068 [details] [review] This is diff-patch containing mentioned changes
(In reply to manasa.athreya from comment #16) > (In reply to Tim-Philipp Müller from comment #15) > > > The following is the case observed: > > > > > > 1. In NONE/raw case the code is not checking the type of endian, which is > > > simply set to Little endian format. > > > > I don't understand what you mean by this. With the current code, no > > endianness is set for NONE/raw, and it's not needed since it's only U8. With > > your patch it's set to BIG_ENDIAN, which is correct according to the > > quicktime specs. With my suggestion to fallthrough to the 'twos' case, it > > would also be set to big endian. > > > Since initially break statement is place after NONE/raw, case was not > going to twos case so we placed code again in NONE/raw. > > > > > 2. The attached file has 16-bit PCM data which is being set to 8-bit data by > > > default. > > > > I understand this. And your patch fixes it. I just asked you to fix it > > differently, with less code. > > > Yes, Will try to fix it in less code. > > > > > 3. stream->bytes_per_packet might not be always 1 in NONE/raw case. > > > > I understand this. And your patch fixes it. I just asked you to fix it > > differently, with less code. > > > > Also: > > > > 4. In the 'twos' case, the data might be 8-bit (S8 in this case) as well. > > The code currently doesn't handle that. > > > > I think the NONE/raw/twos case could be unified into one block. > > I agree to this idea of unifying. > > Also: > I will try to unify and submit the same. I have unified NONE/raw/twos case .Please check the attached patch.
Comment on attachment 308068 [details] [review] This is diff-patch containing mentioned changes Thanks, almost there now, but one issue: In case of NONE/RAW, the 8-bit audio is unsigned (U8), in case of twos/swot, the 8-bit audio is signed (S8) the code needs to handle both. I think you can simplify this even more by simply doing format=... in the 8-bit case and using the caps creation function and the description string creation code in all cases (for the signed audio, the U8 case has to be special-cased before).
Created attachment 308071 [details] [review] The diff patch after refactoring.
(In reply to Tim-Philipp Müller from comment #19) > Comment on attachment 308068 [details] [review] [review] > This is diff-patch containing mentioned changes > > Thanks, almost there now, but one issue: > > In case of NONE/RAW, the 8-bit audio is unsigned (U8), in case of twos/swot, > the 8-bit audio is signed (S8) the code needs to handle both. > > I think you can simplify this even more by simply doing format=... in the > 8-bit case and using the caps creation function and the description string > creation code in all cases (for the signed audio, the U8 case has to be > special-cased before). I have generalized the cases for 8,16 etc bit data while setting caps based on format it will retrieve signed, unsigned and endianness. Please check the attached patch .
Comment on attachment 308071 [details] [review] The diff patch after refactoring. Thanks for your patience :) I think this new patch still has an issue: it makes NONE/raw 8-bit audio S8 instead of U8, if I'm not mistaken. I think to fix this you could move the GstAudioFormat format up to where the endian variable is declared, and init it to 0 or FORMAT_UNKNOWN. Then in the NONE/RAW case you'd set that to U8 if samples_per_byte == 1 and then you fall through to the twos/sowt case. Then later you check if format is already set, and if not use gst_audio_format_build_integer().
Created attachment 308194 [details] [review] This is the diff-patch after modification
Comment on attachment 308194 [details] [review] This is the diff-patch after modification commit e6381ef285ca6e0c909b1ad6cba6183fe1ac435d This patch still has some problems: 1) qtdemux.c: In function 'qtdemux_parse_tree': error: 'depth' may be used uninitialized in this function: format = gst_audio_format_build_integer (TRUE, endian, depth, depth); 2) the indentation was not right, please run gst-indent on the .c file in future. I have fixed these now and pushed this: Author: Manasa Athreya <manasa.athreya@lge.com> Date: Mon Jul 27 13:34:14 2015 +0900 qtdemux: fix 16-bit PCM audio advertised with 'raw ' fourcc 'NONE' and 'raw ' fourcc don't always contain U8 audio, it can be more bits as well, in which case it's just like 'twos'. https://bugzilla.gnome.org/show_bug.cgi?id=752613 I'm not sure why you are talking about 'unsigned 16-bit PCM' in your commit message, it seems to be signed 16-bit PCM.