MIXER palette gets "garbled" after screen change

25 posts / 0 new
Last post
jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
MIXER palette gets "garbled" after screen change

Hi, just updating Mixer V1.33 (os4depot) and added some features :-)
The problem I'm having is with pens (ObtainBestPen) AFTER CHANGING ScreenMode Mixer pens they don't look what they should be, LIGHT_GREEN shows white and alike.

Mixer V1.33:

  1. ..
  2. ObtainColors()
  3. {
  4. ...
  5. PenArray[LIGHT_GREEN] = ObtainBestPen(MainScreen->ViewPort.ColorMap, 0x4DDDDDDD,0xE3333333,0x4DDDDDDD, TAG_DONE);
  6. ..
  7. }

I just set 6 pens/colors and use them on the MixWindow like this:

  1. ..
  2. SetAPen(MixWindow->RPort, PenArray[LIGHT_GREEN]);
  3. ..

If I get a screen_change, Mixer does:

  1. ..
  2. // If our screen was closed or changed, try to reinit
  3. TempScreen = LockPubScreen(NULL);
  4. if(TempScreen != MainScreen)
  5. {
  6. UnlockPubScreen(NULL, TempScreen);
  7. MainScreen = NULL;
  8. MainScreen = LockPubScreen(NULL);
  9. free(DriverNames); // AlexC
  10. DriverNames = NULL; // AlexC
  11. PropsInitiated = FALSE;
  12. initCode = MainInit_lite(); // it calls ObtainColors();
  13. }
  14. else UnlockPubScreen(NULL, TempScreen);
  15. ..

Should I ReleasePens() too, but where (before/after MainScreen=NULL)?

  1. ReleasePens()
  2. {
  3. ..
  4. ReleasePen(MainScreen->ViewPort.ColorMap, PenArray[i]);
  5. }

TIA

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
That's very bad style. You

That's very bad style. You must not allow the screen to close before you didn't ReleasePen.

The correct sequence is

LockPubScreen
Obtain[Best]Pen
OpenWindow

... use pen for drawing ...

CloseWindow (or iconify)
ReleasePen
UnlockPubScreen

While your window is open or the screen is locked, the screen will not close. So no need to compare pointers. And before you close the window and unlock the screen (so that it is allowed to close) you must release the pens.

Keeping the pens allocated and allow the screen to close is very bad style.

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@jabirulo if(TempScreen !=

@jabirulo

if(TempScreen != MainScreen)

EDIT: Also, keeping a global pointer to the screen the program opened on (MainScreen) is bad practice. When you need it, obtain the pointer locally.

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
OK THX , so good code should

OK THX , so good code should be if program is iconified/hidden it should remove/close all stuff related to window/screen like pens, ¿menus (pointers to "old" window/screen)? and such?

TIA

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@jabirulo Yes. Read my

@jabirulo

Yes. Read my screen programming article, it may give you some useful tips. Basically, an OS4 program should never cache screen or window pointers (ie. store them in a global variable). Thanks to the window iconification and screen jumping features, these pointers may become invalid at some point. Therefore, pointer caching introduces a risk that an invalid pointer might get accessed. In order to avoid such a risk, dynamic querying about screen/window pointers and obtaining them for local use only is recommended.

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
THX. I think I removed all

THX. I think I removed all the "naughty" screenlocks. Now time to change'em to use GetScreenAttrs(). And obtain/release pens seems ok now too.

Next work is see/check image loading/disposing. It seems if I remove/rename some of the images Mixer uses, on uniconifying/showing Mixer again it boombs :-P
BTW is it a good idea to lock such files while mixer is showing and unlock them on iconify/hide?
TIA

BTW: can someone show me how to use 'GetScreenAttrs()' instead of 'LockPubScreen()' in this part of code, or is just nonsense and is ok to lock/unlock screen (is a local variable) in this case:

  1. ...
  2. MainScreen = LockPubScreen(NULL);
  3. if( ARG("IMAGEPATH") ) strcpy(NameWithPath, ARG("IMAGEPATH"));
  4. else strcpy(NameWithPath, "Images/");
  5. AddPart(NameWithPath, FileName, 1024);
  6. if( (o = NewDTObject( (char *)NameWithPath,
  7. DTA_GroupID, GID_PICTURE,
  8. OBP_Precision, PRECISION_IMAGE,
  9. PDTA_Remap, TRUE,
  10. PDTA_Screen, MainScreen,
  11. PDTA_FreeSourceBitMap, TRUE,
  12. PDTA_UseFriendBitMap, TRUE,
  13. PDTA_DestMode, PMODE_V43,
  14. TAG_END)) )
  15. {
  16. modeid = GetVPModeID( &(MainScreen->ViewPort) );
  17. if( GetDisplayInfoData(NULL, (APTR)&di, sizeof(struct DisplayInfo),
  18. DTAG_DISP, modeid) )
  19. {
  20. fri.fri_PropertyFlags = di.PropertyFlags;
  21. fri.fri_Resolution = *(&di.Resolution);
  22. fri.fri_RedBits = di.RedBits;
  23. fri.fri_GreenBits = di.GreenBits;
  24. fri.fri_BlueBits = di.BlueBits;
  25. fri.fri_Dimensions.Depth = MainScreen->BitMap.Depth;
  26. fri.fri_Screen = MainScreen;
  27. fri.fri_ColorMap = MainScreen->ViewPort.ColorMap;
  28. }
  29. ...
  30. }
  31.  
  32. UnlockPubScreen(NULL, MainScreen);
  33. ...

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
can someone show me how to

can someone show me how to use 'GetScreenAttrs()' instead of 'LockPubScreen()'

You can't. This part of Trixie's post was probably meant differently (I didn't understand it, too).

Next work is see/check image loading/disposing.

Same as before. You must not UnlockPubScreen() before you did DisposeDTObject(). If you supply a screen pointer to the datatype object, you have to make sure that the pointer remains valid until you free the object.

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@jabirulo can someone show

@jabirulo

can someone show me how to use 'GetScreenAttrs()' instead of 'LockPubScreen()'

Sorry, after a night of heavy coding I wrote complete nonsense :) All I wanted to say is don't keep the screen pointer in a global variable. When you need it, the pointer can be obtained locally from the window object:

  1. struct Screen *scr = NULL;
  2.  
  3. IIntuition->GetAttrs(winObj, WA_PubScreen, (uint32 *)&scr);

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
@Trixie: you are in the wrong

@Trixie: you are in the wrong thread. Mixer does not use ReAction. Therefore there is no WinObj which could be used to retrieve information from. And all the dangers you mentioned just don't exist.

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
OK. Yes Mixer uses

OK. Yes Mixer uses intuition/gadtools :-/ The screen lock/unlock seems solved the way I have it now (no golbal pointers to screen). I can change screenmode without problems.

2 (and half) more questions:
1)Is it safe to use malloc() [newlib i think] or is there an AOS4 substitute?

2)It seems if I remove/rename some of the images (or dir) Mixer uses, on (un)iconifying Mixer GR/boombs :-P as Mixer tries to 'Dispose(object)'.
BTW is it a good idea to lock such files/dir while mixer is showing and unlock them to provent user rename/delete such files/dir while Mixer is running?
2b)Will Lock/UnLock solve me that issue?

TIA

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
As I told you you have to

As I told you you have to DisposeObject before you iconify. Whenever (before) you let go the screen you have to release all objects which were tied to the screen.

I doubt that DisposeObject takes any notice of the file no longer being existent. It only releases allocated memory. Actually NewDTObject keeps the file locked until you DisposeDTObject, so at least deleting the file is not possible. An additional Lock will not help.

It's more likely that it crashes because you changed some resources in memory while you were iconified. For example if you close/reopen the screen, DisposeObject will crash if it tries to release allocated pens.

Before you iconify you have to release all resources which are related to other resources which may change while you are iconified. And on uniconify you have to reacquire all pointers to these other resources.

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
OK. While Mixer is showing

OK. While Mixer is showing its window I can rename/delete images/dir so there is no Lock on such files.

IIRC Mixer does DisposeObject() before iconifying ... well it seems not :-P ('cos it crashes/GR) will have to check such code.

TIA

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@Thomas Mixer does not use

@Thomas

Mixer does not use ReAction. Therefore there is no WinObj which could be used to retrieve information from. And all the dangers you mentioned just don't exist.

Can Mixer iconify/hide? Then caching the screen pointer can potentially be dangerous and requires more care. Same thing whether the program is based on ReAction or GadTools. It's a general principle.

@jabirulo
All screen-related pointers (including GadTools' VisualInfo, for example) must be dropped when the window closes and re-obtained when it reopens. Therefore, it doesn't make any sense to keep them global. That's all I'm trying to say.

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
Hi, added an iconify gadget

Hi, added an iconify gadget via sysclass and ¿buttonclass? (don't remember now).
Changed global screen var to be local (and using only when I have to). Now it (un)inconifies without problem, even can change screenmodes and everything is fine.
The problem with image loading/disposing is solved too, just have to clean/NULL some variables that weren't before.
Will upload Mixer to os4depot ASAP.

BTW THX for all the help! :-)

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
Three cheers for an improved

Three cheers for an improved Mixer!!! :-)

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
:-D Just a "cosmetic" thing

:-D
Just a "cosmetic" thing how to make main window (uses gadtools/intuition) not aware of clicks/mousewheel/etc. when for example showing the about window.
I'm using ' SetWindowPointer(MixWindow, WA_BusyPointer, TRUE, TAG_END);' to change mouse icon, but main window still acts on mouse clicks and such.
Is there some sort of 'SetWindowAttr(MixWindow, SLEEP)'?
Or somekind of SetWindowAttr(MixWindow IDCMP_Flags, NULL); and re-enabling them when about window closes.

TIA.

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
APTR SleepWindow (struct
  1. APTR SleepWindow (struct Window *win)
  2. {
  3. struct Requester *lock = NULL;
  4.  
  5. if (win)
  6. if (lock = AllocMem (sizeof(struct Requester),MEMF_CLEAR|MEMF_PUBLIC))
  7. {
  8. InitRequester (lock);
  9. Request (lock,win);
  10.  
  11. if (win->FirstRequest == lock)
  12. SetWindowPointer (win,WA_BusyPointer,TRUE,TAG_END);
  13. else
  14. {
  15. FreeMem (lock,sizeof(struct Requester));
  16. lock = NULL;
  17. }
  18. }
  19.  
  20. return (lock);
  21. }
  22.  
  23.  
  24. void WakeWindow (struct Window *win,APTR lock)
  25. {
  26. if (lock)
  27. {
  28. EndRequest(lock,win);
  29. FreeMem (lock,sizeof(struct Requester));
  30. SetWindowPointer (win,WA_BusyPointer,FALSE,TAG_END);
  31. }
  32. }

You could change the IDCMP flags with ModifyIDCMP(), but this does not prevent the user from clicking on GUI elements. Only your Window does no longer hear about it.

The method shown above is the "official" way as of AmigaOS version 1.

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
THX thoma, just updated with

THX thoma, just updated with not deprecated OS4.1 functions:

if( (lock = AllocVecTags(sizeof(struct Requester), AVT_Type,MEMF_SHARED, AVT_ClearWithValue,0, TAG_DONE)) )

and

FreeVec(lock);

Hope they are OK. Works flawlessly!!! :-)

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@jabirulo Hope they are

@jabirulo

Hope they are OK.

They're not! Do not use shared memory allocation for a temporary requester structure that will only be used by your own task. The memory will not be shared, so use MEMF_PRIVATE instead.

Also note that shared memory is always locked by default so when allocating with MEMF_SHARED (which is, I repeat, not your situation - you want private memory!) you must either unlock it before deallocating, or allocate with the "AVT_Lock, FALSE" tag (unlocked memory would hamper the system memory layout optimization).

Read more about OS4 Exec memory allocation in the OS4 documentation wiki.

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
Yep, thx I already read it,

Yep, thx I already read it, but as you see didn't understand quite well.
Will read it again and make changes on my code if necessary.
If I use a memory allocation for temporary "objects", I should use MFM_PRIVATE?
OK going to read the wiki again THX.

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@jabirulo The rule is very

@jabirulo

The rule is very simple: if the block of memory is only going to be used by your program, then use MEMF_PRIVATE.

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

thomas
thomas's picture
Offline
Last seen: 1 week 10 hours ago
Joined: 2011-05-16 14:23
But the requester is used by

But the requester is used by input.device. Everything in the GUI is also used by input.device.

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
Two cases from Mixer

Two cases from Mixer sources/code:

1)arexx.c (code to parse Mixer AREXX commands):

  1. if( (ro = AllocVecTags(sizeof(ArexxHandle), AVT_Type, MEMF_SHARED, TAG_DONE)) )

// before it was MEMF_ANY

2)revo51_ctr.c (init/control the audio of soundcard Revo5.1 [Envy24HT]):

  1. RevoFrontCodec = AllocVecTags(sizeof(struct akm_codec), AVT_Type, MEMF_SHARED, TAG_DONE);

// before it was MEMF_ANY

Suggestions? TIA

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

trixie
trixie's picture
Offline
Last seen: 5 months 2 weeks ago
Joined: 2011-02-03 13:58
@thomas But the requester is

@thomas

But the requester is used by input.device. Everything in the GUI is also used by input.device.

I don't believe you'd have to allocate shared memory for such a trivial thing as a data structure defining a GUI element. The AmigaOS Documentation Wiki says that "private memory should always be preferred" - surely it would mention that GUI stuff must be allocated as shared because of input.device?

@jabirulo
Anyway, it's easy to find out: if it crashes with MEMF_PRIVATE then you know you must use MEMF_SHARED :-D

AmigaOne X5000-020 / 2GB RAM / Sapphire Pulse Radeon RX 560 / AmigaOS 4.1 Final Edition Update 2

jabirulo
jabirulo's picture
Offline
Last seen: 2 weeks 1 day ago
Joined: 2013-05-30 00:53
OK THX. So to resume "rule of

OK THX.
So to resume "rule of the thumb" ;-)
Mixer is getting more robust and less buggy. THX for all your help. :-)

AOS4.1/SAM460ex/PPC460EX-1155MHZ/2048MB/RadeonHD6570/SSD120GB/DVDRW :-P

Log in or register to post comments