-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Leigh,

On 5/1/14, 9:22 AM, L. HAYWARD wrote:
> https://github.com/Leighbee13/RUNTHIS/tree/master

For your first web application, you just *had* to use jQuery, eh?

Okay, I've got no jQuery experience, but it looks like when you click
the "Add Song" button, the add_more handler runs:

    $('#add_more').click(function() {
        var current_count;
        if ($('.addsort').length !== 0) {
            current_count =
parseInt($('#inputs').children(':last-child').children().first().attr('name'));
        }else{
            current_count=1;
        }
        var next_count = current_count + 1;
        $('#sortable').append('<li id= "song' + next_count + '"
name="'+next_count+
                '" class="ui-state-default"><span class="ui-icon
ui-icon-arrowthick-2-n-s"></span> <img class="deletesong"
src="images/delete.png" alt="del" width="20" height="20" style="float:
right;"></li>');
        $('#inputs').append('<p><input type="file" id="file_' +
next_count + '" class = "addsort" name ="' + next_count + '"
accept=".wav, .au*" required/></p>');
    });

This is the markup from your JSP:

                    <div id="inputs">
                        <p><button type='button' id="add_more"
style="float: right;">Add Song</button><!--<a id="add_more"
href="#">Add song</a>--></p>
                        <h3>Songs:</h3>
                        <p> <input type="file" class= "addsort"
id="file_1" name ="1" accept=".wav, .au*" required/></p>
                    </div>

If I read the jQuery code correctly, it's going to choose the first
child of the last child of the DIV and use that element's "name" as
the current_count, then add one to it to get the next_count.

I believe the first child of the last child of the DIV is actually the
<button>, which would make the current_count nil, as it has no "name"
attribute. If I've got this wrong, please correct me.

You might want to eliminate the jQuery from the equation and just
hard-code the <form> to have some specific number of file inputs and
just leave some of them blank for testing. You can always check for
null-ness on the server side (and you should, since the user could
always leave one of the dynamically-added ones blank).

Okay, comments on the actual Java code:

1. Do not have doGet call doPost. It's not going to work. Just remove
goGet entirely, and the parent class will return a "Method Not
Supported" error to the client.

2. What does the stdout log usually look like for a "good" run? How
about a "bad" one?

3. If the part.write() call fails, you are screwed. You shouldn't just
(log and) ignore the exception. Instead, you should blow up and tell
the user that something bad happened.

4. What's the "program select" thing? Does that interact with the
other audio clips uploaded? Why are you using a java.util.Scanner?

5. When you call requestDispatcher.forward(), you need to return from
the doPost method ASAP. The way your servlet is written, the rest of
the servlet will continue on running (trying to overlay the songs,
etc.) and chaos will ensue. You can simply put a "return;" after the
call to requestDispatcher.forward(). I'm specifically looking at the
forward() call at line 109.

6. I'd recommend that you use an array of java.io.File objects as the
parameter to your overlay() method, but that's more of a matter of
taste than anything that will affect your program. You could also use
a set of InputStreams and writing the individual audio files to the
disk manually. I'm not sure that would improve anything other than a
bit of a performance gain. It would simplify your code, which is
always a nice thing to do. Feel free to ignore this entire comment, as
it shouldn't change anything about the correctness of your code.

7. You are allocating enormous byte buffers to read-in the audio clips
in their entirety. Is that strictly necessary? Also, you are
truncating long values to int values. For your testing, I assume that
you are never exceeding Integer.MAX_VALUE, but you could be surprised
by something like that. I realize this is "toy code" but if every
dissertation would include decent code, the world would be a better place.

8. You are doing a lot of manipulations across many buffers that could
probably be combined into fewer operations: reading bytes, packing
bytes, summing integers, unpacking bytes, then re-encoding as an audio
clip.

9. Your overlay() method should probably take the name of the target
file as a parameter. That way, the caller will know where the file is
going. Then, provide the same file name to the finish() method. That
way, the target file name is only specified in a single place, and you
can change it whenever you want (like if you want to make this thing
work with multiple users, it needs no further modification if the
doPost method chooses its own unique target file name for each request).

10. In your finish() method, you should buffer the reads and writes to
improve performance. Feel free to use a BufferedInputStream to read
the file, or you could just use your own byte[] buffer. Processing
files one byte at a time sucks. You might think that FileInputStream
and ServletOutputStream would be buffered, but you'd be wrong.

11. In concatenate, you start with songs/noth.wav (an empty WAV file?)
and then you concatenate each file, one at a time, each time, so you
end up with a bunch of files named test0.wav, test1.wav, test2.wav,
etc. each successive one being the contents of the previous one plus
one additional song, right? I think you can shorten the whole thing by
opening all the files at once as AudioInputStreams, then packing them
into an Enumeration, and passing-off the whole thing to
AudioSystem.write, like this:

public void concatenate(String[] songs, String targetFilename)
    throws Exception
{
  long totalFrameCount = 0;

  ArrayList<InputStream> inputs = new ArrayList<>(1 + songs.length);

  AudioInputStream nothing = loadToWav("sound/noth.wav");
  inputs.add(nothing);
  totalFrameCount += nothing.getFrameLength();

  AudioFormat fmt = nothing.getFormat();
  for(String input : songs) {
    AudioInputStream ais = loadToWav(input);
    inputs.add(ais);
    totalFrameCount += ais.getFrameLength();
  }

  SequenceInputStream sis = new
SequenceInputStream(Collections.enumeration(inputs));
  AudioInputStream in = new AudioInputStream(sis, fmt, totalFrameCount);
  AudioSystem.write(in,
                    AudioFileFormat.Type.WAVE,
                    new File(targetFilename));
  in.close();
  sis.close(); // Maybe unnecessary?
}

I have a dumb question: if WAV uses frames, can't you just concatenate
all of the bytes together from the source files and not worry about
AudioInputStreams and stuff like that? I did a bit of Googling and
nobody seems to have said that simply concatenating the bytes of a
bunch of WAV files together will actually work. Is the problem that
you have to make sure they're all of the same type, and then you'd
have to remove the headers as you perform the concatenation? I suppose
it's just easier and less fragile to formally decompress the audio and
then re-encode it. It just seems like kind of a waste.

I do know that you can simply concatenate MP3 files in the way I've
described, because there is no file header. But if you aren't careful,
you'll get noisy pops and stuff when two adjacent frames (at the file
boundary) introduce a discontinuity in the audio.

There's a lot of stuff in there that I don't understand, but I'm
hoping that with a few tweaks, you can get your problem solved.

I'm fairly confident that #5 above is going to fix 90% of the problem
you have been having.

- -chris
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBCAAGBQJTapr2AAoJEBzwKT+lPKRYe6YP/R202RoQtVXdsON407Lw+LXm
L90DlwfSmUpVTAdQu8f07f0T/0FCcB5vuDpS/JfHNdwC7F/p0S90iMyrgdI1oM5S
MHeC0Y+uF/z4cPme7lNvcQA/E1zw9+GC65vUWa0BELW42I563b1IyxEG+Ec+i2nS
a4Zl9ub3gtx3FK1SYtDr93pnIwbFEJgPHynNQxbQcBlbeclaLIwLdDYd7UUIkCyc
tJkIhtXsmOYGp/bmG3lZHrgUj7RjR9AztEl61jWxItFpg7oWC6gJI34CTOCNqCjl
r2qOtLbBSWVnwBtNoKs2e5Ve01tIWs9sWamZhre8GpitDv3kj+ODu7qe/a5TWJup
YNs/VgVr/w62P/UVhpo5Je6ml3AqRXrWt40QH+kxcIPeOc6Y7E3ELIbtUGLzLnQW
+b2BwSUfi9yFaYjxvmKVbri7vCvs0I857F8pY/BDN3t+s7+nkgRfB+JTQwmqb22/
SlVTQ4JJfnkurUyu/iu/BQB2KcdjFPuUTtLXSO0/OHnEGq1RZZy5AiLE2U9Wyfxm
bHvTHsCMmelcLVdRSAi2ul7HVUvTZgZPVgGO7lPzAdy4qSN8OK+g8yDjeQAS3Jz3
Zj/+Gh3mhD2Bgf+Bc9CTE/2YKtasZ5tbWvW/1HiQ/uwyMT2uMPbLvSfZvoIbb3ba
Mo0XovV3o0v/IR2JZ9u+
=7Tww
-----END PGP SIGNATURE-----

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

Reply via email to