Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gr-audio: update comments in wavfile_sink and wavfile_source and add list of supported audio files #6503

Closed
wants to merge 378 commits into from
Closed

Conversation

harshiiiit
Copy link

@harshiiiit harshiiiit commented Feb 2, 2023

Made changes to comments in wavfile_sink and wavfile_source and added list of supported audio files.

Signed-off-by: Harshit Singh singhh198@gmail.com

Description

Related Issue

Fixes #5971

Checklist

@harshiiiit harshiiiit changed the title Made changes to comments in wavfile_sink and wavfile_source Made changes to comments in wavfile_sink and wavfile_source and added list of supported audio files. Feb 2, 2023
@willcode willcode changed the title Made changes to comments in wavfile_sink and wavfile_source and added list of supported audio files. gr-audio: update comments in wavfile_sink and wavfile_source and add list of supported audio files Feb 2, 2023
@willcode
Copy link
Member

willcode commented Feb 2, 2023

The list of audio containers/format might just want to be "all supported by libsndfile". That way we don't have to keep the comment up to date. Please amend the commit message so it's like the PR title - that makes it clearer what module it applies to. Thanks.

@willcode willcode added the Backport-3.10 Should be backported to 3.10 label Feb 2, 2023
@willcode
Copy link
Member

willcode commented Feb 2, 2023

Please squash the commits. Also run clang-format -i on files that were changed to make sure the format check passes. Make sure that each commit (there will be only one) contains your Signed-off-by: so the DCO check passes. I know this seems like a lot of work for a simple comment change, but you'll be familiar with the process for bigger changes later!

@willcode
Copy link
Member

willcode commented Feb 2, 2023

I went back and checked in the code (wavfile.h) and here are the actual supported formats:

enum wavfile_format_t {
    FORMAT_WAV = 0x010000,
    FORMAT_FLAC = 0x170000,
    FORMAT_OGG = 0x200000,
    FORMAT_RF64 = 0x220000,
};

In the grc yml file it's expressed as:

-   id: format
    label: Output Format
    dtype: enum
    options: [FORMAT_WAV, FORMAT_FLAC, FORMAT_OGG, FORMAT_RF64]
    option_labels: [WAV, FLAC, Ogg Vorbis, 64-bit WAV]

So, neither of us had the right list!

@harshiiiit
Copy link
Author

Okay, I will make these changes. But I couldn't understand the part clang-format -i. Can you help me out with how to do that?

@willcode
Copy link
Member

willcode commented Feb 2, 2023

If you changed gr-blocks/include/gnuradio/blocks/wavfile_sink.h, run
clang-format -i gr-blocks/include/gnuradio/blocks/wavfile_sink.h

argilo and others added 19 commits February 3, 2023 02:09
This avoids a race condition which causes messages to be skipped while
the flow graph finishes execution.

Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Josh Morman <jmorman@gnuradio.org>
Format *_c_pydoc_template.h and *_python.cc using clang-format
to make the generated files clang_format compliant.

It's better to use clang-format than to modify the mako templates.
Using clang-format directly you have nothing to do when changing the formatting
rules but only run the binding process again, but you must not change the mako templates.

As with 956f332
pywidget() was replaced qwidget() generic_python_cc.mako had to be modified, to generate the correct
binding code.

Signed-off-by: Volker Schroer <3470424+dl1ksv@users.noreply.github.com>
Valgrind reports that poly_pack reads uninitialized memory, leading to
undefined behaviour. This happens because it rounds the length up to
the next multiple of 32 before reading the polynomial coefficients.
Initializing the coefficients to zero solves the problem.

Signed-off-by: Clayton Smith <argilo@gmail.com>
This fixes undefined behaviour (left shift of a negative number)
reported by gcc's Undefined Behaviour Sanitizer.

Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
The qa_filterbank test occasionally fails, and from the error message
it's apparent that on these occasions outdata2 contains the same values
as outdata. This suggests that the while loop that waits for the new
filter taps to kick in is exiting too soon. I suspect this occurs
because it compares floating point values for exact equality. I've
changed this to check for approximate equality instead.

Signed-off-by: Clayton Smith <argilo@gmail.com>
When running QA tests, gcc's Undefined Behaviour Sanitizer reports
signed integer overflows in fxpt.h. I've fixed them by explicitly
casting the input to an unsigned integer prior to the addition step.

Signed-off-by: Clayton Smith <argilo@gmail.com>
gcc's Undefined Behaviour Sanitizer reports load of a misaligned
address. Replacing the offending code with a memcmp fixes the issue.

Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
In #5804 I accidentally caused a compiler warning:

warning: suggest braces around initialization of subobject [-Wmissing-braces]

This change fixes the warning while still initializing the polyout
array to zero.

Signed-off-by: Clayton Smith <argilo@gmail.com>
This brings crc16_async_bb in line with crc32_async_bb, making it easier
to compare them.

Signed-off-by: Clayton Smith <argilo@gmail.com>
The CRC16 and CRC32 async blocks can perform unaligned reads during
verification, which results in undefined behaviour. To fix this, I've
switched them to use memcmp.

The CRC16 validation was also incorrect, because it read two extra
bytes, resulting in verification failures. I've fixd that here as well.

Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Ron Economos <w6rz@comcast.net>
Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
Signed-off-by: Clayton Smith <argilo@gmail.com>
argilo and others added 24 commits February 3, 2023 02:09
Signed-off-by: Clayton Smith <argilo@gmail.com>
Change gr::digital::ofdm_frame_equalizer_vcvc block to pass the tags to
the equalizer that it aggregates. The gr::digital::ofdm_equalizer_base
interface allows passing the tags vector to equalize() but in current
implementation the default argument is used (empty vector).

Fixes #6422
Signed-off-by: Mihai Stef <mihai.stef@com.utcluj.ro>
np.float as an alias for float has been deprecated and removed, and
np.float_ or np.float64 is undoubtedly what was wanted here anyway.

Signed-off-by: Ryan Volz <ryan.volz@gmail.com>
Signed-off-by: Josh Morman <jmorman@gnuradio.org>
Allow new snippet insertion point in the init function but before blocks
are instantiated.  This allows actions or objects that might be required
in the constructors of the blocks, such as connecting to a server, to be
initialized

Signed-off-by: Josh Morman <jmorman@gnuradio.org>
Importing gtk before requiring a specific version can cause issues
if multiple versions of gtk are installed on a system.

Signed-off-by: Seth Hitefield <sdhitefield@gmail.com>
Signed-off-by: Seth Hitefield <sdhitefield@gmail.com>
Signed-off-by: Jeff Long <willcode4@gmail.com>
Signed-off-by: Josh Morman <jmorman@gnuradio.org>
I have described the issue here

#6459
Signed-off-by: Fran Boric <fboric@yahoo.com>
Signed-off-by: theartful <aessam.dahy@gmail.com>
Signed-off-by: Jeff Long <willcode4@gmail.com>
Signed-off-by: Grant Meyerhoff <grant.meyerhoff@ni.com>
Signed-off-by: Grant Meyerhoff <grant.meyerhoff@ni.com>

Co-authored-by: Martin Braun <martin.braun@ettus.com>
Co-authored-by: Grant Meyerhoff <grant.meyerhoff@ni.com>
bytes_read, the result of asio::ip::udp::socket::receive_from(), was
an int, but the function returns size_t. This resulted in an overflow
of the int and a subsequent comparison.

Since the int result of buffer space_available() should never be negative,
it is safe to cast the result to size_t. If that result is ever changed
to something larger than size_t, this could cause trouble, but that seems
unlikely.

Signed-off-by: Jeff Long <willcode4@gmail.com>
3eea105
causes the gr-soapy block to add the rx_freq tag when set_frequency() is called with a name parameter.

However, gr-soapy source's cmd port handler calls set_frequency() without name, so add_tag_ is not
set and the rx_freq tag is not added. This patch causes rx_freq to be added for both versions of set_frequency().

Signed-off-by: Josh Bailey <josh@vandervecken.com>
This fixes a bug in the calculation of the frame size used by the
tagged decoder that affects the CC decoder in terminated mode.

Currently, tagged_decoder calls the set_frame_size() method of the
decoder with a size computed as the input size multiplied by the
rate.

In the cc_decoder, the frame_size parameter of the
set_frame_size() method is used to calculate and set d_veclen. The
value of d_veclen is used to determine how many forward steps
(butterflies) the Viterbi algorithm should be run. The number of
input items that are read in d_veclen forward steps is d_veclen * d_rate
(note that d_rate is actually the inverse of the rate; typically,
d_rate == 2). Therefore, the size of the input buffer should be as
large as this value, and in most cases it should be equal, because we
want the cc_decoder to look at all the provided input.

The d_veclen variable is related to d_frame_size, which indicates
how many output bits are produced by the decoder. The value of
d_frame_size is equal to the parameter of set_frame_size() in all cases
except in terminated mode with byte padding, in which case the byte
padding is subtracted when computing d_frame_size. The value of d_veclen
is equal to d_frame_size + d_k - 1 in terminated and streaming modes,
and to d_frame_size in truncated mode.

The problem with the current code is that in tagged_decoder the
set_frame_size() method is called with ninput_items[0] * rate. In the
streaming and terminated modes this will cause d_veclen to be too
large. The forward steps (butterflies) of the Viterbi algorithm will
read past the end of the input buffer. This is undefined behaviour and
can cause some bit errors at the end of the decoded codeword (the effect
of reading past the end of the buffer is essentially extending the
codeword with 12 symbols of garbage).

This problem does not happen in the async_decoder because there is
some code that computes a "diff", which works out to be
  d_k - 1 + d_padding * rate()
in terminated mode and 0 in all other cases. The diff is subtracted
from the frame size. This subtraction gives the correct d_veclen
calculation, so that the butterflies do not read past the input end.

This commit fixes the tagged_decoder when used with the terminated CC
decoder by adding the same code as in the async_decoder to calculate
and subtract this "diff" value.

Some caveats:

1) This commit changes the size of the output packets of tagged_decoder
when used with a terminated CC decoder. Formerly, the output packets
were 6 bits longer, as if the tail bits were included in the output.
However, having these 6 extra bits was just an artifact of looking 12
symbols past the end of the input. The cc_decoder_impl::chainback_viterbi()
function is not prepared to extract tail bits in the terminated case.
This chainback extracts bits "as they exit the shift register on the
right", so tail bits cannot be extracted, because they "never exit the
shift register". (Here I have in mind the usual depiction of a convolutional
encoder, where input bits are shifted from left to right into a shift
register).

The CC_TRUNCATED case in cc_decoder_impl::generic_work() has some
additional code to extract additional bits which haven't "exited
the shift register" yet, because it is mandatory to extract them in this
case (they are the last 6 bits of the message).

It think there is no good use case for attempting to extract the tail
bits in the terminated case, so this modification seems a good way forward.
The new behaviour also matches that of async_decoder, which does not output
tail bits.

However, this breaks tag propagation of the tagged_decoder when used
with the terminated CC decoder, because the block calls set_relative_rate()
with the nominal rate (1/2), which is slightly different from the true
rate once we take into account the fact that tail bits are dropped. This
could be fixed by propagating tags manually in the work() function.

2) The streaming mode is still broken, both when used with the
async_decoder and with the tagged_decoder. Since for the streaming case
diff is 0 but d_veclen includes the term d_k - 1, the streaming mode
reads past the input buffer. It isn't really possible to run the
streaming mode properly with the async_decoder or tagged_decoder
(it should be run with the decoder_impl.cc), because the streaming mode
requires history (some overlapping input items between consecutive
calls), which is not available with the async_decoder or
tagged_decoder.

3) The code copied from async_decoder has a "watch out" comment stating
that it might be over-specializing for the CC decoder in terminated
mode. It is true that this code seems specially crafted to cover this
very specific case, but as far as I have been able to think, it does
not affect negatively any other cases (taking into account other modes
of the CC decoder and other FEC decoders).

Signed-off-by: Daniel Estévez <daniel@destevez.net>
Signed-off-by: R-ohit-B-isht <rbtunes0@gmail.com>
all these seem copy file-and-modify oversights; looks like no harm was done.

Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
Not required:
e36f2fa ("pdu: Remove unused dependencies", 2022-06-01)

Signed-off-by: Chris Mayo <aklhfex@gmail.com>
Signed-off-by: R-ohit-B-isht <rbtunes0@gmail.com>
…list of supported audio files

Signed-off-by: harshitwr <singhh198@gmail.com>

Signed-off-by: harshitwr <singhh198@gmail.com>
@drmpeg
Copy link
Member

drmpeg commented Feb 2, 2023

Just a note that the Opus format was added. See this commit for changes:

eaeb378

@drmpeg
Copy link
Member

drmpeg commented Feb 2, 2023

Format support is asymmetrical. The source block will try to read all of the libsndfile supported formats, but the sink block only selects a subset.

Also, mp3 support (SF_FORMAT_MPEG) is only in libsndfile version 1.1.0, which has not made it into most distros (will be in Ubuntu 23.04).

In addition, the comment in wavfile_source.h about only supporting 8 or 16 bits is no longer true.

…list of supported audio files. Signed-off-by: Harshit Singh <singhh198@gmail.com>

Signed-off-by: harshitwr <singhh198@gmail.com>
@harshiiiit
Copy link
Author

I am sorry I made a mistake. Going to solve it.

@harshiiiit harshiiiit closed this Feb 3, 2023
@harshiiiit harshiiiit deleted the wavfile_documentation branch February 3, 2023 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wavfile Sink: Header docstring still only mentions PCM, but we do all of libsnd