After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752613 - qtdemux: raw 16 bit PCM audio in 'raw ' fourcc not working
qtdemux: raw 16 bit PCM audio in 'raw ' fourcc not working
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.5
Other Linux
: Normal minor
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 752760
 
 
Reported: 2015-07-20 08:43 UTC by manasa.athreya
Modified: 2015-08-16 13:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
This is the test file to be used to reproduce bug (843.23 KB, video/quicktime)
2015-07-20 08:43 UTC, manasa.athreya
  Details
The above is the diff patch code. (1.59 KB, patch)
2015-07-20 08:45 UTC, manasa.athreya
none Details | Review
This is split file part whose md5 sum is e476e8c5f12498d1dff4735732900aa5 (1.00 MB, video/quicktime)
2015-07-23 03:42 UTC, manasa.athreya
  Details
This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5 (1.00 MB, application/octet-stream)
2015-07-23 03:42 UTC, manasa.athreya
  Details
This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5 (185.30 KB, image/jpeg)
2015-07-23 03:43 UTC, manasa.athreya
  Details
This is the diff-patch. (1.96 KB, patch)
2015-07-24 05:47 UTC, manasa.athreya
none Details | Review
This is diff-patch containing mentioned changes (1.94 KB, patch)
2015-07-24 09:47 UTC, manasa.athreya
none Details | Review
The diff patch after refactoring. (1.05 KB, patch)
2015-07-24 10:54 UTC, manasa.athreya
none Details | Review
This is the diff-patch after modification (1.75 KB, patch)
2015-07-27 04:40 UTC, manasa.athreya
needs-work Details | Review

Description manasa.athreya 2015-07-20 08:43:30 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
Comment 1 manasa.athreya 2015-07-20 08:45:17 UTC
Created attachment 307731 [details] [review]
The above is the diff patch code.
Comment 2 Tim-Philipp Müller 2015-07-20 10:31:05 UTC
Did you attach the right test file?

The test file contains AAC, not PCM audio.
Comment 3 manasa.athreya 2015-07-20 10:33:55 UTC
(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.
Comment 4 manasa.athreya 2015-07-20 11:06:19 UTC
My file size is 2234KB ,unable to upload. Can u please let me know how to upload large files?
Comment 5 Tim-Philipp Müller 2015-07-20 11:20:20 UTC
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).
Comment 6 Tim-Philipp Müller 2015-07-22 11:33:57 UTC
Feel free to just send me the file by e-mail if it's just ~2MB.
Comment 7 manasa.athreya 2015-07-23 03:42:05 UTC
Created attachment 307955 [details]
This is split file part whose md5 sum is e476e8c5f12498d1dff4735732900aa5
Comment 8 manasa.athreya 2015-07-23 03:42:57 UTC
Created attachment 307956 [details]
This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5
Comment 9 manasa.athreya 2015-07-23 03:43:55 UTC
Created attachment 307957 [details]
This is split file part whose md5sum is e476e8c5f12498d1dff4735732900aa5
Comment 10 manasa.athreya 2015-07-23 03:44:59 UTC
Thanks Tim, I have attached files by splitting them with there md5sum.
Comment 11 Tim-Philipp Müller 2015-07-23 20:38:44 UTC
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.
Comment 12 manasa.athreya 2015-07-24 03:51:44 UTC
Tim,Did you mean that, I should get developer access for the patch and submit the changes?
Comment 13 manasa.athreya 2015-07-24 05:47:32 UTC
Created attachment 308050 [details] [review]
This is the diff-patch.
Comment 14 manasa.athreya 2015-07-24 05:55:51 UTC
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.
Comment 15 Tim-Philipp Müller 2015-07-24 08:08:36 UTC
> 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.
Comment 16 manasa.athreya 2015-07-24 08:38:20 UTC
(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.
Comment 17 manasa.athreya 2015-07-24 09:47:50 UTC
Created attachment 308068 [details] [review]
This is diff-patch containing mentioned changes
Comment 18 manasa.athreya 2015-07-24 09:49:20 UTC
(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 19 Tim-Philipp Müller 2015-07-24 09:59:41 UTC
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).
Comment 20 manasa.athreya 2015-07-24 10:54:30 UTC
Created attachment 308071 [details] [review]
The diff patch after refactoring.
Comment 21 manasa.athreya 2015-07-24 10:56:36 UTC
(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 22 Tim-Philipp Müller 2015-07-24 11:16:04 UTC
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().
Comment 23 manasa.athreya 2015-07-27 04:40:50 UTC
Created attachment 308194 [details] [review]
This is the diff-patch after modification
Comment 24 Tim-Philipp Müller 2015-07-27 18:12:22 UTC
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.