Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

White space for forcing carriage return in a paragraph #77

Merged
merged 1 commit into from Jan 9, 2018

Conversation

JeromeMartinez
Copy link
Contributor

When I filled the spec with carriage returns in a paragraph with Markdown, my goal was to output a carriage return in HTML. Actually not the case.
This commit fixes that, adding carriage returns.
Reference for how to put a carriage return in Markdown: https://markdown-guide.readthedocs.io/en/latest/basics.html#line-return

Example of issue: all on the same line on https://github.com/FFmpeg/FFV1/blob/master/ffv1.md#quant_table_set_count, split in 3 lines on https://github.com/MediaArea/FFV1/blob/Markdown/ffv1.md#quant_table_set_count.

@michaelni
Copy link
Member

Why do you want to force new lines in the middle of paragraphs ? depending on how much width there is, that may be sub optimal

@JeromeMartinez
Copy link
Contributor Author

I think that:

quant_table_set_count indicates the number of Quantization Table Sets. Inferred to be 1 if not present. MUST NOT be 0.

is less readable as:

quant_table_set_count indicates the number of Quantization Table Sets.
Inferred to be 1 if not present.
MUST NOT be 0.

As each line is a different part of the definition. Having a different paragraph for each line would be too much.

another problematic example is:
https://github.com/FFmpeg/FFV1/blob/master/ffv1.md#initial_state_delta

initial_state_delta [ i ][ j ][ k ] indicates the initial Range coder state, it is encoded using k as context index and pred = j ? initial_states[ i ][j - 1][ k ] : 128 initial_state[ i ][ j ][ k ] = ( pred + initial_state_delta[ i ][ j ][ k ] ) & 255

It is very difficult to read (for me: it is impossible to understand, I just see a single formula with two = and I don't understand the link between each part), with pred definition and initial_state[ i ][ j ][ k ] definition on the same line, without any separator, but the source is:

initial_state_delta [ i ][ j ][ k ] indicates the initial Range coder state, it is encoded using k as context index and
pred = j ? initial_states[ i ][j - 1][ k ] : 128
initial_state[ i ][ j ][ k ] = ( pred + initial_state_delta[ i ][ j ][ k ] ) & 255

From my point of view the current version (all on 1 line, same separator between a part of the formula and another formula, i.e. space) is not understandable as a definition of of pred then a definition of initial_state.

This is definitely very subjective, we can abandon this change if nobody else thinks it is useful. The opposite idea would be to remove the carriage returns in the Markdown source as the Markdown version makes thinking that there is a carriage return where there is no carriage return.

Maybe at least keeping modification for formulas (pred / initial_state formulas issue)?

@michaelni
Copy link
Member

From my point of view the current version (all on 1 line, same separator between a part of the formula and another formula, i.e. space) is not understandable as a definition of of pred then a definition of initial_state.

The formular in your example a bit slow to parse in both cases. There are 4 things the text conveys to the reader, 1. what it is, 2. what is stored (delta mod 8 from some predictor), 3. the predictor, 4. the context used for the coder. I agree the newline makes it better in this case but it still intermingles things and isnt very clear

@dericed
Copy link
Contributor

dericed commented Aug 27, 2017

Status of this? It appears that @JeromeMartinez's addition of spacing for line breaks makes it more readable. @michaelni points out that it could be more readable, but that could occur in a subsequent commit and doesn't have to block this.

@dericed dericed mentioned this pull request Jan 2, 2018
@JeromeMartinez JeromeMartinez deleted the Markdown branch January 3, 2018 18:41
@JeromeMartinez JeromeMartinez restored the Markdown branch January 4, 2018 08:11
@JeromeMartinez JeromeMartinez reopened this Jan 4, 2018
@JeromeMartinez
Copy link
Contributor Author

Sorry, I inadvertently deleted the branch.
Pinging about this PR, if not acceptable a is I would like to have hints about what would be acceptable.
My opinion is similar to the one from Dave (changing even more in order to be more readable would be part of another PR, this one is for carriage returns only).

@michaelni
Copy link
Member

this doesnt apply cleanly anymore

@JeromeMartinez
Copy link
Contributor Author

Branch rebased.

@michaelni michaelni merged commit dee437f into FFmpeg:master Jan 9, 2018
@JeromeMartinez JeromeMartinez deleted the Markdown branch March 29, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants