michaelni changed the topic of #ffmpeg-devel to: Welcome to the FFmpeg development channel | Questions about using FFmpeg or developing with libav* libs should be asked in #ffmpeg | This channel is publicly logged | FFmpeg 8.0 has been released! | Please read ffmpeg.org/developer.html#Code-of-conduct
<Traneptora>
ramiro: no, makernote is just generally garbage. spec says it's a binary blob. but some manufacturers (e.g. Canon) make it an IFD which means if you move it around it corrupts it
<Traneptora>
so we first try to parse it as an IFD and if that doesn't work for any reason, treat it as binary blob
<BtbN>
I've never before seen a comment that just has _no_ patch though, but that is all handled by the same code, which should now not have a path through it anymore that doesn't outright panic that leaves the comment without a context
<BtbN>
Though not entirely sure how a comment would already end up with no context whatsoever.
<BtbN>
The main issue was that it's always picking HEAD of the PR, not the commit that's being reviewed.
<mkver>
BtbN: When I review line x in file in a commit of a PR, then the "Files changed" shows the diff of the whole PR together with comments that apply to the diff shown (i.e. if output line x (in head of the PR) would be shown, then the comment would be shown on that page. If the comment does not pertain to a line (according to the incorrect linenumber) that would not be shown anyway, then no comment appears. And in this case the
<mkver>
main PR site also has an empty context (but at least the comment is shown).
<BtbN>
the main page should not have an empty context though
<BtbN>
it knows which commit was being reviewed and should pull the patch snippet from it
<mkver>
Yes, I just wanted to share my observation with you wrt why some (and which) comments don't have context on the main page.
<mkver>
Of course I am not arguing that the current behavior is how it should be.
<BtbN>
Did it happen in this case though, that the HEAD of the PR lost the file(s) that were being commented in?
<mkver>
No. All my comments apply to lavc/webp.c
<BtbN>
weird
<BtbN>
I guess we will first have to wait for 12.0.5, and see how the fix for the most glaring bug interacts with it all
<BtbN>
Maybe it's already enough, or maybe there's more hidden somewhere