Frame class and it's API

[musteresel]

I'm currently struggling a bit with Frame's API.

https://openshot.org/files/libopenshot/classopenshot_1_1Frame.html

(I'll try to summarize the issues I have, but it may take a few edits over the next hours / days even :) )

We have constructors:

• a default constructor
• an image only constructor
• an audio only constructor
• image and audio construtor

We have public data members:

• two flags for whether there's audio / image data
• the frame number

There are member functions to "Add" to the frame:

• AddAudio
• AddAudioSilence
• AddColor
• AddImage
• AddMagickImage

There are member functions to "query" (sometimes not only ...) stuff of the frame:

• ChannelsLayout
• CheckPixel
• GetAudioChannelsCount
• GetAudioSample
• GetAudioSampleBuffer
• GetAudioSamples
• GetAudioSamplesCount
• GetBytes
• GetWidth
• GetHeight
• GetPixels
• GetPixelRatio
• GetImage
• GetMagickImage
• GetPlanarAudioSamples
• GetInterleavedAudioSamples
• GetSamplesPerFrame
• GetWaveform
• GetWaveformPixels
• SampleRate

Then there are a few "modifying" member functions:

• SampleRate
• SetFrameNumber
• SetPixelRatio
• ResizeAudio
• ChannelsLayout
• ApplyGainRamp

Finally there are member functions which "do" things, e.g. for debugging:

• ClearWaveform
• DeepCopy
• Display
• DisplayWaveform
• Play
• Thumbnail

I hope I didn't forget a member function.

All in all this is ... a lot .. for a "container" of audio and image data. Furthermore, some of these functions do more than they're advertising, e.g. GetPixels() creating image data if there's none, whereas GetPixels(0) /* a row */ just crashes when there's no image data.

Which of these functions are actually used? Or rather: Where should I look for the "main" user of frames? Which (undocumented) side effects are actually used? Is Frame used directly from OpenShot?

I think Frame would greatly benefit from a bit cleanup work :)

Related: #446

[ferdnyc]

A few things I would characterize differently:

• DeepCopy is used by the assignment operator (and I think by the copy constructor? I forget, do we have a copy constructor?), so it's a (necessary) utility method, in my view, not a debugging tool. It could be private, perhaps, arguably.
• ClearWaveform is a modifying method, if GetWaveform is a query method. (Though GetWaveform, like some other methods you noted, also causes the data it's requesting to be created if it doesn't exist.)
• AddMagickImage and GetMagickImage sort of belong to a special category of their own, because they only exist if the library is linked with ImageMagick. In a build without the Magick libs, those methods won't be there at all. (Make of that what you will, in terms of the API... it's just a point of distinction.)

It's a good deal of API, but I guess it's a question of opinion whether it's "a lot" for the purpose. The audio data handling adds a significant amount of complexity, probably more than the image data handling actually. (Especially if you include the waveform image processing.) But if you compare it to something like Qt's QImage, for example... and that's JUST image data.

API consumers

I don't recall whether any part of OpenShot directly consumes Frames, but if not that's only because it sticks to higher-level APIs (Clip, FFmpegWriter, etc) built on it. And, all of libopenshot is built on Frames. They're fundamental to everything from the Reader and Writer classes (which produce and consume them, respectively) to the effects system (which are passed Frames to operate on and return modified), to the cache (for which it is a basic unit of data).

Utility methods (Thumbnail, primarily) also aren't used directly by OpenShot, iirc, but they are heavily RELIED upon by OpenShot. This is from memory, but IIRC OpenShot's Timeline WebView triggers (via the HTTP server, now) a thumbnail request for a given frame# made via the Clip API, which in turn creates a Frame object for that frame# and calls Frame::Thumbnail() to write the image file.

There are also other libopenshot consumers (OpenShot is not the only application built on it) which may make far more direct use of Frame -- frankly I just don't know, but point is even the question "what does OpenShot use?" isn't sufficient to gauge the API's external dependents.

Switching gears

I personally HATE the side-effectey nature of Frame. IMHO it causes problems even within the libopenshot code. Actually... come to think of it, that's not even my opinion. It's a proven fact. There is/was at least one OpenShot bug (where clips with disabled video would show up as black squares in the layer-composite output — no longer 1px × 1px squares, they were scaled to the Timeline frame dimensions) that was a direct result of the Frame API creating data where none existed or was supposed to exist.

It's also a near certainty that past and existing memory leaks in the library are attributable to sloppy use of Frame. (Coupled with its own internal use of std::shared_ptr<QImage> — an ill-advised combining of two very different approaches to implementing thread-safe memory management, Qt's implicit data sharing and the C++11 STL's smart pointers, that very likely may confuse or cancel each other out.)

So, yeah, if I could have my way I'd change Frame a lot, but my focus wouldn't be on the API (except maybe to get rid of methods like Play and Display that aren't API calls at all, really), it'd be on the underlying implementation. Starting with the elimination of its primary misfeature, the automatic creation of image data for otherwise-blank frames. In an alpha-composited, multi-layered video model, a BLANK frame is one that contains either no image data, or (at most) a fully transparent image -- NOT a 1px × 1px black square.

[musteresel]

So, yeah, if I could have my way I'd change Frame a lot, but my focus wouldn't be on the API (except maybe to get rid of methods like Play and Display that aren't API calls at all, really), it'd be on the underlying implementation. Starting with the elimination of its primary misfeature, the automatic creation of image data for otherwise-blank frames.

Hm, yes that hits the nail on the head. The API ("list of publicly accessible member functions") isn't so much my problem I guess but more the underlying side effects ("getting" pixels shouldn't create some) and the (probably) dependency on these "undocumented side effects" from multiple places within libopenshot itself.

I've stumbled over this while trying to create more tests for Timeline ... tests which don't rely on some undocumented side effects.

[ferdnyc]

and the (probably) dependency on these "undocumented side effects" from multiple places within libopenshot itself.

There might actually be less of that than you'd think. The impression I get is that those side-effect "features" were added to make the API more forgiving, when coding against it. IOW, you don't have to worry about checking whether a frame has image data, you can just request it and you'll always get something in return. A nice idea, if misguided. But not really for the benefit of libopenshot itself, more for external consumers.

If anything, the side-effects seem to trip up libopenshot as well, more often than not. (As in that bug I mentioned where clips with disabled video show up as black squares.)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.