Thursday, 2023-08-17

*** Danct12 is now known as Guest912102:40
mal@b100dian check the droidmedia PR comments, something to fix12:17
T42<TheVancedGamer> mal: could you comment on my pa-droid PR, please?12:21
malthat parsing PR?12:22
T42<TheVancedGamer> mal: yes12:23
T42<b100dian> Thanks mal, looking12:31
mal@b100dian just to clarify that latter comment is about a possibly missing include of the header12:53
T42<b100dian> yes, it was very clear, thank you12:53
T42<b100dian> mal: pushed13:00
mal@b100dian still failed but different way, "fatal error: unordered_map: No such file or directory" maybe it uses wrong c++ standard during build or something13:13
malneed to check which bases have that issue and maybe we need some "-std=c++11" to flags in for older bases13:14
T42<b100dian> When that fails, you have the whole command line used to build?13:15
malunfortunately no13:16
malI'm doing some test builds with different bases to see which ones fail13:17
T42<b100dian> I see audiopolicy_9 to 11 headers use the same include13:22
mal@b100dian external/droidmedia/droidmediabuffer.cpp:131:22: error: could not convert 'buffer->_DroidMediaBuffer::m_queue' from 'android::sp<_DroidMediaBufferQueue>' to 'bool'13:25
maladd != NULL to the f13:25
malto the if13:25
malI'm still checking which android bases need fixes for the header13:26
T42<b100dian> Thanks, that's easy, I'll push if you have an update on the gcc side (android version that fails).13:28
mal@b100dian looks like android 6 doesn't show issues with the include (unless that failure happens to hide the issue) but android 5 does13:28
T42<b100dian> And we also support 4.4 I would guess? Or even lower 4.x?13:28
mal4.4 is probably the oldest we still care about, try adding that -std=c++11 to LOCAL_CPPFLAGS for all modules in for android versions <= 513:30
mallet's see if that helps13:30
maland fix that if condition13:31
T42<b100dian> Ok, pushed :fingerscrossed:13:36
mal@b100dian looking better, android 6 and 8 bases built fine now, 5 is still building13:48
T42<b100dian> I've only now seen the "for all modules".  Hopefully only for libdroidmedia is needed, as private.h/cpp are not reused in other places13:51
mal@b100dian the parameter is "-std=c++11" not with --13:54
malnot sure if that is the reason why 5 base still failed13:56
T42<b100dian> Oh.. sorry about that. Updated.13:56
malstill failed, I will test locally to see how to really fix that14:03
mallet's see if I have some android 5 base somewhere14:04
T42<b100dian> Oh, so you are remoting somewhre. I was going to ask, what are you building to test Android 5. Something like Nexus 4?14:05
malI was using internal obs for test builds14:07
mal@b100dian try adding LOCAL_SHARED_LIBRARIES += libc++ and LOCAL_C_INCLUDES += external/libcxx/include14:40
T42<b100dian> for <=5, in addition to -std=c++11 I would guess14:41
malyes, only for <= 514:42
maland keep the -std=c++1114:42
T42<b100dian> thanks for figuring this out mal14:43
T42<b100dian> pushed14:43
malthis is still a guess based on grepping android sources and checking ldd of libdroidmedia on different devices :)14:44
malI had removed android 5 sources locally to get more free space14:44
malneed to sync those again if I can figure out the fix otherwise14:45
malanother option is to just ifdef those parts of code you added so those would not be used on <=5, let's se3e14:46
T42<b100dian> I see, yes, I could try that next14:47
mal@b100dian still failed to build, maybe it's just easier to disable the new changes for Android <= 515:01
T42<b100dian> Ok, pushed - but probably requires re-review and re-testing on my side. I will be able to do that later today15:10
malthanks, I'll make test builds15:25
mal@b100dian now both Android 5 and 6 built without issues, probably newer work also15:31
malI'll test on devices later today15:39
T42<b100dian> I will test too and squash if succesful. Thanks for all the help with the various build environments!15:51
mal@b100dian a tiny comment for the future, when making PRs please make sure your own fork has all tags from sfos repo, makes testing easier17:04
malin this case this is not a big issue but if something depends on the package then obs won't install correct version when building the packages which depend on the changed package17:04
malwait, I think you had the tags, somehow misread the version17:08
mal@b100dian some issues when testing, I get "decStrong() called on 0x78780022a0 too many times"17:23
malin droid_media_buffer_destroy you call releaseBufferResources which does decStrong and then again do the same17:24
malthis causes app to abort after playback of any video17:26
mal@b100diando you have some device which doesn't have the original issue, I think your code only works on your device17:40
T42<b100dian> mal: I will test the latest change on lineage -17.1 base where I didnt have the original issue. But I did install the previous version (before ifdefs) on that port and there werent issues.18:00
T42<b100dian> mal: what do you mean by "and then again do the same", inside that method I tried to introduce an early return when adding the ifdefs18:16
T42<b100dian> I mean I know you're right since you have the issue but I can't figure it out18:16
mal@b100dian I mean in that droid_media_buffer_destroy function you call releaseBufferResources(buffer) which calls decStrong for the buffer, then in droid_media_buffer_destroy you call decStrong again for the buffer18:23
malI mean it happens if unslotted doesn't have the buffer18:24
malit's possible different android bases don't always make that issue critical18:27
malso either you rethink how those are called or check the ref count like I suggest some time ago using getStrongCount, assuming that is in all bases18:31
T42<b100dian> mal: sorry I don't have the external storage to build the other port and test. There are only two decStrong calls in the PR. One is made when m_queue is null. The other when the buffer is not passed to the "unslotted" map. The buffer is passed in to the unslotted map when a situation like I encountered on lineage-18.1 is happening, and that is again calling releaseBufferResources before adding the buffer.20:05
T42<b100dian> Without being able to debug, I *think* that two decStrong calls can happen when at the end of playback the m_queue is null. That codepath was preserved for .. compatiblity reasons, but I might just remove the else from if(m_queue != NULL)20:05
T42<b100dian> I pushed what I think would be removal of that "compatibility path" and decStrong should only be called from releaseBufferResources and should not be called twice at end of video if m_queue being null is indeed the cause. If that doesn't solve it I need a couple of days to be next to the external tucana 17.1 harddrive. Thanks20:10
T42<b100dian> Droid media buffers are not sp per se, they dont have getRefCount directly, byw - I looked into that a month ago20:27
T42<b100dian> *btw, I did looked that up a month ago20:44
mal@b100dian it there any way m_queue can be null and buffer still needs decStrong?20:46
T42<b100dian> I initially left that as there was no m_queue need in the original code. But your testing implies that there is a case where m_queue is null. I hope that the releaseBuffers callback comes before that still. Again, I am sorry that I cannot reproduce at the moment your findings.20:50
T42<b100dian> The device whose build I have at hand clearly has m_unslotted buffers by the end of the playback, but regular (existing) devices clearly don't move buffers to m_unslotted so..20:52
mal@b100dian did misunderstand my case, my case was when m_queue is not null and unslotted didn't contain my buffer21:36
malyour latest code only does decStrong the buffer is m_queue is not null and m_unslotted doesn't contain the buffer, I just wonder will the buffer retain the count in your case now? I wonder if there is now a case where the count never gets to 021:43
mal@b100dian still crashes, I'm trying to see why21:51
mal@b100dian I wonder about the reordering you did in buffersReleased should we change the order back to how it was i.e. buffers_released call to last?22:36
malI think I might know the issue, I'll tell it tomorrow if you are around23:38

Generated by 2.17.1 by Marius Gedminas - find it at!