Create Oscillogram Timeline

[azfoo]

closes #93: created audio image timeline

[FelipeDefensor]

Ok, I glanced at it, and you really seem to have found your way around the codebase. Bravo!

Unfortunately, I'm getting a bug right away (which is a shame because I'm really excited to see this working). When I try to load a media (local or YouTube) the duration in the toolbar (and I imagine in other parts of the software) is not updated, I still get a 0:00.0/0:00.0. Probably because of that, I'm unable to create any timelines, since TiLiA checks for duration != 0 to see if a media is loaded and having a media loaded is a requirement for creating a timeline.

[azfoo]

Oh dear. I don't think this iteration of the timeline touches anything that deals with loading and unloading media. The timeline has to be manually added after media is loaded. Any clue on how to reproduce the error?

[FelipeDefensor]

Yeah, I agree. I checked the code and, indeed, nothing related to loading the media seems to be modified.

On my end, I just need to run main.py and tried loading a media via File > Load media. I suppose that doesn't reproduce the bug on your end. Maybe try a fresh clone of the repo?

[FelipeDefensor]

Ok, so now I noticed this error message:

QtWarningMsg: No QtMultimedia backends found. Only QMediaDevices, QAudioDevice, QSoundEffect, QAudioSink, and QAudioSource are available. QtWarningMsg: Failed to initialize QMediaPlayer "Not available"

[FelipeDefensor]

Ok, so I upgraded to PyQt 6.7 on the upstream repo (i.e. the "original", not your fork) and was able to reproduce the bug, so this is most likely a PyQt issue. I'm having trouble downgrading though, so I'm still unable to run your code.

Does upgrading to the latest PyQt also breaks things on your end?

[azfoo]

So I managed to reproduce the error in a virtual environment, with only the packages from requirements.txt. Could it be the missing ffmpeg that is causing this to happen?

[FelipeDefensor]

Good call, this does seem to be ffmpeg related. If we set the environment variable QT_MEDIA_BACKEND to windows, the media loads fine (the default value is ffmpeg). As far as I know, an ffmpeg executable is supposed to be bundled with PyQt, so this is still weird.

[FelipeDefensor]

I'm getting a blank oscillogram, though.

I wonder if that's also ffmpeg related. Are we relying on ffmpeg for the oscillogram?

[azfoo]

Interesting... The image is created using pydub which relies on ffmpeg (the documentation suggests it's for exporting audio, but I suspect reading audio would require it too).

Could you show the terminal output starting from the beginning?

[FelipeDefensor]

Yeah, I think it relies on ffmpeg for doing what we want. I downloaded the ffmpeg binaries and added then to PATH and now the timeline is working.

[FelipeDefensor]

It looks great, btw. Performance also seems to more than enough. Great job.

[FelipeDefensor]

Ok, so I got the first real bug, I think. When I click and drag the oscillogram bars, I get an exception like this:

Traceback (most recent call last):
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\ui\timelines\view.py", line 86, in mouseMoveEvent
    post(Post.TIMELINE_VIEW_LEFT_BUTTON_DRAG, event.pos().x(), event.pos().y())
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\requests\post.py", line 221, in post
    callback(*args, **kwargs)
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\ui\timelines\drag.py", line 38, in on_mouse_drag
    self.after_each(dragged_to)
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\ui\timelines\oscillogram\element.py", line 109, in after_each_drag
    self.set_data("time", get_time_by_x(drag_x))
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\ui\timelines\base\element.py", line 53, in set_data
    self.timeline_ui.set_component_data(self.id, attr, value)
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\ui\timelines\base\timeline.py", line 194, in set_component_data
    self.timeline.set_component_data(id, attr, value)
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\timelines\base\timeline.py", line 165, in set_component_data
    return self.component_manager.set_component_data(id, attr, value)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\timelines\base\timeline.py", line 245, in set_component_data
    value, success = self.get_component(id).set_data(attr, value)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\timelines\base\component.py", line 50, in set_data
    if not self.validate_set_data(attr, value):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Felipe\dev\tilia-desktop-azfoo\tilia\timelines\base\component.py", line 39, in validate_set_data
    raise SetComponentDataError(
tilia.exceptions.SetComponentDataError: Component 'Oscillogram-2514084572304' has no attribute named 'time'. Can't set to '38.13435655253836'.

I'm guessing you partially implemented dragging unknowingly, so TiLiA is trying to set the timeproperty of the oscillogram component when I click and drag. There seems to be no such property, though.

[FelipeDefensor]

Second thing, changing the timeline's height is not working properly. Instead of resizing the graph, the graph is getting cropped.

You probably need to reimplement TimelineUI.update_height to fix that.

Also, the default height of 240 is excessive. Remember that there will be several other timelines timelines on-screen. I suggest we try out something like 80, to see how it looks.

[FelipeDefensor]

So, I think we're back on track. I'll do a more detailed review over the weekend or early next week. After we finish implementing this timeline, I think that ensuring that ffmpeg is bundled properly with TiLiA or installed on-demand has become a top priority.

[azfoo]

Thanks for identifying those issues! I have fixed both bugs with the latest commits. Let me know if you find anything else.

And yes, I agree with the need to sort out ffmpeg...

[FelipeDefensor]

Thanks for identifying those issues! I have fixed both bugs with the latest commits. Let me know if you find anything else.

And yes, I agree with the need to sort out ffmpeg...

Regarding this, I tested the new code and evrything works fine now.

[azfoo]

Importantly, they look just like a point-by-point plot when zoomed out and still capture the essential information when zoomed in.

Have you tried pressing refresh after zooming in to the point of noticing bars?

(1) We need some tests. Probably not many, as there is little interaction from the user with the timeline. Do you think you can handle creating those?

Will look into that

(2) I think the name of the timeline should be something more colloquial. I understand that oscillogram is the technical term, but I'm imagining a slightly less specialized user. What do you think about "audio wave timeline"?

That would work too. I was looking for a more concise name since I didn't think user would be looking too far into the code. But renaming it to that would work

[FelipeDefensor]

Have you tried pressing refresh after zooming in to the point of noticing bars?

Oh, I dindn't try that. So the "resolution" increases (up to a limit) if we do that, right?

That would work too. I was looking for a more concise name since I didn't think user would be looking too far into the code. But renaming it to that would work

That's fair. But there are some important user contact points with the name, like the menu option for creating the timeline and references to it in the help docs. We could use the user-friendly name just in those points, but I think having two names would make coding harder.

We're going with AudioWaveTimeline then, right? Please change the class names accordingly. We will need a new name for the each vertical bar also, don't you think? Perhaps AmplitudeMarker or WaveMarker? Not sure I like those, though.

[azfoo]

Oh, I dindn't try that. So the "resolution" increases (up to a limit) if we do that, right?

Yes. The only point which it would be very very necessary is probably when you have a long file with short quiet parts that disappear with the original slices

We're going with AudioWaveTimeline then, right? Please change the class names accordingly. We will need a new name for the each vertical bar also, don't you think? Perhaps AmplitudeMarker or WaveMarker? Not sure I like those, though.

Will do. Perhaps SoundBar or AmplitudeBar?

[FelipeDefensor]

Will do. Perhaps SoundBar or AmplitudeBar?

I like AmplitudeBar. How do you like SoundChunk or AudioChunk?

[azfoo]

I like AmplitudeBar. How do you like SoundChunk or AudioChunk?

I think AmplitudeBar is probably more descriptive than SoundChunk or AudioChunk

[FelipeDefensor]

I like AmplitudeBar. How do you like SoundChunk or AudioChunk?

I think AmplitudeBar is probably more descriptive than SoundChunk or AudioChunk

Agree. Let's go with that.

[FelipeDefensor]

Having looked at the code, I'm pretty sure we don't need the button and, therefore, this request. I'll make a commit soon with a fix.

On 03e1f55 I was able to trigger a refresh once new media loads. I will also do that for zooming. When that's done, I think the refresh button can be dispensed with. Correct?

[azfoo]

Having looked at the code, I'm pretty sure we don't need the button and, therefore, this request. I'll make a commit soon with a fix.

On 03e1f55 I was able to trigger a refresh once new media loads. I will also do that for zooming. When that's done, I think the refresh button can be dispensed with. Correct?

Looks right! Doing it with zoom levels would probably not work because of how long it takes to clear and redraw the bars. Zooming would start to lag as it is not an asynchronous process and redrawing would happen for every recorded step as opposed to only redrawing on the final zoom level change. Perhaps we ignore redrawing on the zoom levels instead? I don't think too much information will be lost without redrawing.

[FelipeDefensor]

Having looked at the code, I'm pretty sure we don't need the button and, therefore, this request. I'll make a commit soon with a fix.

On 03e1f55 I was able to trigger a refresh once new media loads. I will also do that for zooming. When that's done, I think the refresh button can be dispensed with. Correct?

Looks right! Doing it with zoom levels would probably not work because of how long it takes to clear and redraw the bars. Zooming would start to lag as it is not an asynchronous process and redrawing would happen for every recorded step as opposed to only redrawing on the final zoom level change. Perhaps we ignore redrawing on the zoom levels instead? I don't think too much information will be lost without redrawing.

Yes, you are right. I did some testing and the performance hit is too big. I'm worried it's too big even in the case where there is no clearing and recreating. Maybe we should reduce max_div to significantly less than 7500, beacuse of that.

[azfoo]

Update: I thought it would be better if the timeline gets deleted if there isn't any relevant media to display. Deleting it would remove clutter on the UI, as opposed to leaving it on and not showing anything at all. This will break existing tests because the timeline removes itself as the tests do not actually provide any media.

[FelipeDefensor]

Update: I thought it would be better if the timeline gets deleted if there isn't any relevant media to display. Deleting it would remove clutter on the UI, as opposed to leaving it on and not showing anything at all. This will break existing tests because the timeline removes itself as the tests do not actually provide any media.

That seems sensible, but I have feeling that it will be hard pull off. Regarding the problem you ran into, you could exclude the timeline from those tests, I would have no problem with that.

But consider the case where we're changing from one media to another (which is rare, I reckon). First, the current media gets unloaded. A Post.PLAYER_MEDIA_UNLOADED gets fired. The audio wave timeline receives it and gets deleted. Right after, a new media gets loaded, but there's no audio wave timeline to refresh anymore.

What I feel could work is hiding the timeline if there's no media available. Also, we could draw a bunch of tiny centered bars, as if the audio was silent, I would guess that's the simpler solution.

[azfoo]

I seem to have been able to delete the timeline successfully. I thought hiding the timeline might cause the user to try and make more of them, so deleting them might be better.

[FelipeDefensor]

Here's a Beethoven sonata zoomed in to display a pretty large main theme. It should be very rare that users would find zooming in more than this useful.

The bar spacing looks acceptable, to me. I use 2500 as a fixed div number for this one. The performance when zooming is also acceptable. What do you think about that number?

One thing I will do to help is to increase the zoom factor, so each zoom triggered makes more of a visual difference and less zooming steps are required overall.

[FelipeDefensor]

I seem to have been able to delete the timeline successfully. I thought hiding the timeline might cause the user to try and make more of them, so deleting them might be better.

If you can handle the case where a different media gets loaded mentioned above, I have no problem with going this way.

[FelipeDefensor]

Those last three commits all look good to me.