Range Coder and Golomb Rice pseudo-code formatting #196

JeromeMartinez · 2020-03-31T20:18:29Z

From IETF review, The "type" column here appears superfluous in theses locations.
ur and sr namings are clarified by splitting the pseudo-code in 2 parts + a name for each part.
log2_run is actually an array.

JeromeMartinez · 2020-04-28T08:17:51Z

bump ;-)

michaelni · 2020-04-28T15:59:23Z

Other codec specifications from mpeg4 asp to h265 put pseudocode like if() interspaced with bitstream stuff in tables. So iam a bit reluctant to apply this as it would move us away from that "look"
ive applied the other commit

JeromeMartinez · 2020-05-25T16:08:13Z

I close this PR for the moment and send this feedback to IETF reviewers.

JeromeMartinez · 2020-05-27T15:55:05Z

hmmm... I reopen this PR, for the remaining commit proposal, as I was actually not myself convinced while preparing my answer to AD review.

We have 2 different parts impacted about this "type" column, "Sample Coding" and "Bitstream".

"Sample Coding" is not explicitly describing the bitstream step by step, it is more a description with the help of some tables for the algorithm, so "type" column is not useful in this section. Actually, we already have "diff" and "get_vlc_symbol" definitions without the "type" column so it is incoherent. The commit proposal makes a coherent "look" to the "Sampling Coding" section.
"Bitstream" section is about the bitstream, and as in H265 we have some tables with no element read from the bitstream (Line() for example), and this commit proposal does not remove the "type" column from such table.

@michaelni please comment/review again.

michaelni · 2020-06-21T11:47:13Z

put_symbol() doesnt fit very well to get_ur_golomb.

JeromeMartinez · 2020-06-21T16:46:19Z

put_symbol() doesnt fit very well to get_ur_golomb.

I don't get it, could you explain a bit more?

michaelni · 2020-06-21T18:41:52Z

put_symbol() doesnt fit very well to get_ur_golomb

I don't get it, could you explain a bit more?

one is write and one is read, that is not consistant

michaelni · 2020-06-21T18:42:41Z

also its the bitstream and reading that is specified so it should all be read

JeromeMartinez · 2020-06-22T11:06:14Z

one is write and one is read, that is not consistant

The PR was intended for formatting only, without change in the content (planned in a future PR after this one is merged).
Anyway I added a new commit which replaces put_symbol definition by get_symbol, please review again.

retokromer

I suggest to unify either to

if ( ... ) {
    something;
}

or to:

if ( ... )
    something;

but not to mix the two coding styles.

JeromeMartinez · 2020-06-22T11:31:50Z

I suggest to unify either to

This was my goal, but I forgot at one place, if I miss nothing else. Fixed.

retokromer · 2020-06-22T11:47:47Z

I’m sorry to be picky, but here reads:

}
else {

and in other places:

} else {

JeromeMartinez · 2020-06-22T13:55:21Z

}
else {

Fixed.

dericed · 2020-06-22T14:35:59Z

ffv1.md

 ```
 Figure: A pseudo-code description of the contexts of Range Non Binary Values. {#figureRangeNonBinaryValueExample}

-`put_symbol` is used during the process of (#coding-of-the-sample-difference).
+`get_symbol` is used during the process of (#coding-of-the-sample-difference).


This becomes rendered as:

"get_symbol" is used during the process of Section 3.8.

but currently we are in a subsection of Section 3.8 so the reference feels circular.

but currently we are in a subsection of Section 3.8 so the reference feels circular.

Not really circular, the idea is to link the introduction of this section.
I added a commit for referencing the figure in the introduction of this section, for being more precise.

dericed · 2020-06-22T14:37:57Z

ffv1.md


-`put_rac` is the process described in (#range-binary-values).
+`get_rac` is the process described in (#range-binary-values).


This is rendered as

"get_rac" is the process described in Section 3.8.1.1.

But Section 3.8.1.1 refers to several processes. Could we add a "Figure" label to the process you mean and cross-reference that rather than the whole section. For instance, see how {#figureRelativeSampleNames} is cited.

But Section 3.8.1.1 refers to several processes.

IMO get_rac is for all figures in the section, which describe a single process, so the link is correct. I don't see a better way to link, @michaelni would you have a suggestion here about how to improve this description?

I think that no modification is necessary here, and all other issues are fixed, please review again.

dericed · 2020-06-22T14:38:37Z

ffv1.md

+    return get_bits(bits) + 11
+}
+```
+Figure: A pseudo-code description of the read of an unsigned integer in Golomb Rice mode


Other Figure labels are terminated with ., I suggest that here too.

Other Figure labels are terminated with ., I suggest that here too.

Fixed

dericed · 2020-06-22T14:38:42Z

ffv1.md

 }
 ```
+Figure: A pseudo-code description of the read of a signed integer in Golomb Rice mode


Other Figure labels are terminated with ., I suggest that here too.

Other Figure labels are terminated with ., I suggest that here too.

Fixed

retokromer · 2020-06-23T09:45:52Z

ffv1.md

+int get_ur_golomb(k) {
+    for (prefix = 0; prefix < 12; prefix++) {
+        if (get_bits(1)) {
+            return get_bits(k) + (prefix << k)


add a ; to make the pseudo-compiler happy? (also at 613; it’s done at 621-622)

add a ; to make the pseudo-compiler happy? (also at 613; it’s done at 621-622)

Modified (not "fixed" because it was in the current version :-p).

JeromeMartinez force-pushed the SampleDifference branch 2 times, most recently from d98818e to 69da236 Compare May 25, 2020 16:07

JeromeMartinez closed this May 25, 2020

JeromeMartinez reopened this May 27, 2020

JeromeMartinez force-pushed the SampleDifference branch from 8a6eef2 to eab78e7 Compare June 22, 2020 11:02

retokromer reviewed Jun 22, 2020

View reviewed changes

JeromeMartinez force-pushed the SampleDifference branch from eab78e7 to 7fb55b3 Compare June 22, 2020 11:31

JeromeMartinez force-pushed the SampleDifference branch from 7fb55b3 to 73704e0 Compare June 22, 2020 13:54

dericed reviewed Jun 22, 2020

View reviewed changes

JeromeMartinez force-pushed the SampleDifference branch from 73704e0 to 5370432 Compare June 23, 2020 09:08

retokromer reviewed Jun 23, 2020

View reviewed changes

JeromeMartinez added 3 commits June 23, 2020 11:49

Range Coder and Golomb Rice pseudo-code formatting

1f12fd9

Define get_symbol instead of put_symbol

3203aef

More precise link between get_symbol and sample_difference

22dc258

JeromeMartinez force-pushed the SampleDifference branch from 5370432 to 22dc258 Compare June 23, 2020 09:50

retokromer approved these changes Jun 23, 2020

View reviewed changes

michaelni merged commit 22dc258 into FFmpeg:master Jun 23, 2020

JeromeMartinez deleted the SampleDifference branch June 23, 2020 12:56

dericed mentioned this pull request Jun 23, 2020

few instances of lines too long #198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Range Coder and Golomb Rice pseudo-code formatting #196

Range Coder and Golomb Rice pseudo-code formatting #196

JeromeMartinez commented Mar 31, 2020

JeromeMartinez commented Apr 28, 2020

michaelni commented Apr 28, 2020

JeromeMartinez commented May 25, 2020

JeromeMartinez commented May 27, 2020

michaelni commented Jun 21, 2020

JeromeMartinez commented Jun 21, 2020

michaelni commented Jun 21, 2020

michaelni commented Jun 21, 2020

JeromeMartinez commented Jun 22, 2020

retokromer left a comment

JeromeMartinez commented Jun 22, 2020

retokromer commented Jun 22, 2020

JeromeMartinez commented Jun 22, 2020

dericed Jun 22, 2020

JeromeMartinez Jun 23, 2020

dericed Jun 22, 2020

JeromeMartinez Jun 23, 2020

dericed Jun 22, 2020

JeromeMartinez Jun 23, 2020

dericed Jun 22, 2020

JeromeMartinez Jun 23, 2020

retokromer Jun 23, 2020

JeromeMartinez Jun 23, 2020


		`put_rac` is the process described in (#range-binary-values).
		`get_rac` is the process described in (#range-binary-values).

Range Coder and Golomb Rice pseudo-code formatting #196

Range Coder and Golomb Rice pseudo-code formatting #196

Conversation

JeromeMartinez commented Mar 31, 2020

JeromeMartinez commented Apr 28, 2020

michaelni commented Apr 28, 2020

JeromeMartinez commented May 25, 2020

JeromeMartinez commented May 27, 2020

michaelni commented Jun 21, 2020

JeromeMartinez commented Jun 21, 2020

michaelni commented Jun 21, 2020

michaelni commented Jun 21, 2020

JeromeMartinez commented Jun 22, 2020

retokromer left a comment

Choose a reason for hiding this comment

JeromeMartinez commented Jun 22, 2020

retokromer commented Jun 22, 2020

JeromeMartinez commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment