| *** Danct12 is now known as Guest9121 | 02:40 | |
| mal | @b100dian check the droidmedia PR comments, something to fix | 12:17 |
|---|---|---|
| T42 | <TheVancedGamer> mal: could you comment on my pa-droid PR, please? | 12:21 |
| mal | that parsing PR? | 12:22 |
| T42 | <TheVancedGamer> mal: yes | 12:23 |
| T42 | <b100dian> Thanks mal, looking | 12:31 |
| mal | @b100dian just to clarify that latter comment is about a possibly missing include of the header | 12:53 |
| T42 | <b100dian> yes, it was very clear, thank you | 12:53 |
| T42 | <b100dian> mal: pushed | 13: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 something | 13:13 |
| mal | need to check which bases have that issue and maybe we need some "-std=c++11" to flags in Android.mk for older bases | 13:14 |
| T42 | <b100dian> When that fails, you have the whole command line used to build? | 13:15 |
| mal | unfortunately no | 13:16 |
| mal | I'm doing some test builds with different bases to see which ones fail | 13:17 |
| T42 | <b100dian> I see audiopolicy_9 to 11 headers use the same include | 13: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 |
| mal | add != NULL to the f | 13:25 |
| mal | to the if | 13:25 |
| mal | I'm still checking which android bases need fixes for the header | 13: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 does | 13:28 |
| T42 | <b100dian> And we also support 4.4 I would guess? Or even lower 4.x? | 13:28 |
| mal | 4.4 is probably the oldest we still care about, try adding that -std=c++11 to LOCAL_CPPFLAGS for all modules in Android.mk for android versions <= 5 | 13:30 |
| mal | let's see if that helps | 13:30 |
| mal | and fix that if condition | 13:31 |
| T42 | <b100dian> Ok, pushed :fingerscrossed: | 13:36 |
| mal | @b100dian looking better, android 6 and 8 bases built fine now, 5 is still building | 13: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 places | 13:51 |
| mal | @b100dian the parameter is "-std=c++11" not with -- | 13:54 |
| mal | not sure if that is the reason why 5 base still failed | 13:56 |
| T42 | <b100dian> Oh.. sorry about that. Updated. | 13:56 |
| mal | still failed, I will test locally to see how to really fix that | 14:03 |
| mal | let's see if I have some android 5 base somewhere | 14: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 |
| mal | I was using internal obs for test builds | 14:07 |
| mal | @b100dian try adding LOCAL_SHARED_LIBRARIES += libc++ and LOCAL_C_INCLUDES += external/libcxx/include | 14:40 |
| T42 | <b100dian> for <=5, in addition to -std=c++11 I would guess | 14:41 |
| mal | yes, only for <= 5 | 14:42 |
| mal | and keep the -std=c++11 | 14:42 |
| T42 | <b100dian> thanks for figuring this out mal | 14:43 |
| T42 | <b100dian> pushed | 14:43 |
| mal | this is still a guess based on grepping android sources and checking ldd of libdroidmedia on different devices :) | 14:44 |
| mal | I had removed android 5 sources locally to get more free space | 14:44 |
| mal | need to sync those again if I can figure out the fix otherwise | 14:45 |
| mal | another option is to just ifdef those parts of code you added so those would not be used on <=5, let's se3e | 14:46 |
| T42 | <b100dian> I see, yes, I could try that next | 14:47 |
| mal | @b100dian still failed to build, maybe it's just easier to disable the new changes for Android <= 5 | 15:01 |
| T42 | <b100dian> Ok, pushed - but probably requires re-review and re-testing on my side. I will be able to do that later today | 15:10 |
| mal | thanks, I'll make test builds | 15:25 |
| mal | @b100dian now both Android 5 and 6 built without issues, probably newer work also | 15:31 |
| mal | I'll test on devices later today | 15: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 easier | 17:04 |
| mal | in 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 package | 17:04 |
| mal | wait, I think you had the tags, somehow misread the version | 17:08 |
| mal | @b100dian some issues when testing, I get "decStrong() called on 0x78780022a0 too many times" | 17:23 |
| mal | in droid_media_buffer_destroy you call releaseBufferResources which does decStrong and then again do the same | 17:24 |
| mal | this causes app to abort after playback of any video | 17:26 |
| mal | @b100diando you have some device which doesn't have the original issue, I think your code only works on your device | 17: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 ifdefs | 18:16 |
| T42 | <b100dian> I mean I know you're right since you have the issue but I can't figure it out | 18: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 buffer | 18:23 |
| mal | I mean it happens if unslotted doesn't have the buffer | 18:24 |
| mal | it's possible different android bases don't always make that issue critical | 18:27 |
| mal | so 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 bases | 18: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. Thanks | 20:10 |
| T42 | <b100dian> Droid media buffers are not sp per se, they dont have getRefCount directly, byw - I looked into that a month ago | 20:27 |
| T42 | <b100dian> *btw, I did looked that up a month ago | 20:44 |
| mal | ok | 20: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 buffer | 21:36 |
| mal | your 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 0 | 21:43 |
| mal | @b100dian still crashes, I'm trying to see why | 21: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 |
| mal | I think I might know the issue, I'll tell it tomorrow if you are around | 23:38 |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!